Segher:
On 7/24/24 11:47 AM, Segher Boessenkool wrote:
Hi!
On Wed, Jul 24, 2024 at 11:38:11AM -0700, Carl Love wrote:
On 7/24/24 10:03 AM, Segher Boessenkool wrote:
So much manual stuff needed, sigh.
On Fri, Jul 19, 2024 at 01:04:12PM -0700, Carl Love wrote:
gcc/ChangeLog:
* config/rs6000/altivec.md (vs<SLDB_lr>db_<mode>): Change
define_insn iterator to VEC_IC.
From VI2 (a nothing-saying name) to VEC_IC (also a nonsensical name).
Maybe VEC_IC should have a comment explaining the TARGET_POWER10 thing
at least? Just something like "ISA 3.1 added 128-bit things" or
whatever, but don't leave the reader second-guessing, a reader will
often guess wrong :-)
I don't disagree that the reader will guess wrong, probably after being
frustated that it isn't obvious. :-)
The VEC_IC was an existing definition, this patch does not add it. Your
comments seems to imply you want a comment on the definition for VEC_IC
in vector.md? I could add one to the existing definition if you like
but it seems outside the scope of this patch.
Yes, I'm just lamenting the state of things :-)
It would have to be a separate patch, yes. A trivial patch to add such
a comment is pre-approved :-)
OK, no changes in this patch. Will do a second patch. Best that I post
it for review anyway.
The change log entry could be improved to say "Change define_insn
iterator to VEC_IC which included the V1TI type added in ISA 3.1." Would
that address your concerns?
The current changelog is fine. Changelogs never can replace comments in
the code.
OK
+/* { dg-do run { target { int128 } && { power10_hw } } } */
Everything power10 is int128 always.
OK, so don't need the power10_hw. Changed to just int128 for the target:
No, the other way around: you cannot run the code on machines without
these (ISA 3.1) instructions!
OK, made it { dg-do run { target power10_hw } } */
But p10 always satisfies the int128 predicate. Although, hrm, how
about -m32 :-)
If I test with -m64 I get 8 passes:
make -j 1 && make check-gcc RUNTESTFLAGS="-v -v
powerpc.exp=vec-shift-double-runnable-int128.c --target_board=unix'{-m64}'"
# of expected passes 8
If I test with -m32 I get unsupported test,
make -j 1 && make check-gcc RUNTESTFLAGS="-v -v
powerpc.exp=vec-shift-double-runnable-int128.c --target_board=unix'{-m32}'"
# of unsupported tests 1
Looking further into the output the checks say:
Checking pattern "hppa*-*-hpux*" with powerpc64le-unknown-linux-gnu
compiler exited with status 1
output is:
cc1: error: '-m32' not supported in this configuration^M
check_cached_effective_target power10_hw_available: returning 0 for
unix/-m32
is-effective-target: power10_hw 0
Looks like the power10_hw is sufficient to prevent the test from
running. Don't need to explicitly check for int128 as well.
/* { dg-do run target int128 } */
+/* { dg-do link { target { ! power10_hw } } } */
+/* { dg-require-effective-target power10_ok } */
So this is enough always.
Often we have two testcases, one for run, one for compiling only. It's
a bit simpler and cleaner.
Sounds like you would prefer to have a run and a compile test file? I
will create a new file vec-shift-double-int128.c consisting of a series
of functions to test each built-in definition.
No, I don't prefer that, but it is easier to handle (also for you).
That that results in a bit more files, who cares, I don't anyway :-)
+/* { dg-options "-mdejagnu-cpu=power10 -save-temps" } */
Why the -save-temps? Always document it if you want that for something,
but never put it in the testcase if not. A leftover from development?
Okay for trunk, thank you! Well Peter had some comments too, modulo
those I guess, I'll read them now ;-)
So as Peter said, the save-temps is because the runnable case file also
checks for assembler times at the end of the file.
Yup. A comment would help :-)
OK, added comment.
/* { dg-require-effective-target power10_ok } */
/* Need -save-temps for dg-final scan-assembler-times at end of test. */
/* { dg-options "-mdejagnu-cpu=power10 -save-temps" } */
Carl
