Segher:
Thanks for the review, a few questions...
On 7/24/24 10:03 AM, Segher Boessenkool wrote:
Hi!
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.
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?
gcc/testsuite/ChangeLog:
* gcc.target/powerpc/vec-shift-double-runnable-int128.c: New test
file.
Please don't line-wrap where not wanted. Changelog lines are 80
character positions wide. (Or 79 if you want, but heh).
Yea, it does look like file will just fit on the same line. Fixed.
+The above instances are extension of the exiting overloaded built-ins
(existing)
Fixed spelling error.
a/gcc/testsuite/gcc.target/powerpc/vec-shift-double-runnable-int128.c
b/gcc/testsuite/gcc.target/powerpc/vec-shift-double-runnable-int128.c
new file mode 100644
index 00000000000..bb90f489149
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/vec-shift-double-runnable-int128.c
@@ -0,0 +1,349 @@
+/* { 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:
/* { 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.
+/* { 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.
I will move the scan-assembler-times checks to the new compile only test.
Carl
