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 :-)
> 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.
> >>+/* { 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!
But p10 always satisfies the int128 predicate. Although, hrm, how
about -m32 :-)
> /* { 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 :-)
Segher
