Hi Segher,
Thanks for the review!
>>> Why does this test has_arch_pwr9 instead of adding -mdejagnu-cpu=power9?
>>
>> I thought using -mdejagnu-cpu=power9 would force the case run with
>> power9 cpu all the time, while using has_arch_pwr9 seems to be more
>> flexible, it can be compiled with power9 or later (like -mcpu=power10),
>> we can check whether we generate unexpected code on power10 or later.
>> Does it sound good?
>
> It will not run at all if your compiler (or testsuite invocation) does
> not use at least power9. Since the default for powerpc64-linux is
> power4, and that for powerpc64le-linux is power8, this will happen for
> many people (not to mention that it is extra important to test the
> default setup, of course).
>
Good point! has_arch_pwr9 can cause fewer test coverage if the default
arch is less than power9.
> It probably would be useful if there was some convenient way to say
> "use at least -mcpu=power9 for this, but some later cpu is fine too" --
> but there is no such thing yet.
>
> Using something like that might cause more maintenance issues later, see
> "pstb" below for example, but that is not really an argument against
> fixing this.
Yeah, thanks for the good example!
>> + if (TARGET_POWERPC64)
>> + {
>> + op[i] = gen_reg_rtx (DImode);
>> + emit_insn (gen_zero_extendqidi2 (op[i], tmp));
>> + }
>> + else
>> + {
>> + op[i] = gen_reg_rtx (SImode);
>> + emit_insn (gen_zero_extendqisi2 (op[i], tmp));
>> + }
>> + }
>
> TARGET_POWERPC64 should be TARGET_64BIT afaics? (See below.)
Yes, fixed.
>
> You can use Pmode then, too. The zero_extend thing can be handled by
> changing
> (define_insn "zero_extendqi<mode>2"
> to
> (define_insn "@zero_extendqi<mode>2"
> (and no other changes needed), and then calling
> emit_insn (gen_zero_extendqi2 (Pmode, op[i], tmp));
> (or so is the theory. This might need some other changes, and also all
> other gen_zero_extendqi* callers need to change, so that is a separate
> patch if you want to try. This isn't so bad right now.)
Will deal with this in a separate patch.
>
>> + for (i = 0; i < n_elts; i++)
>> + {
>> + vr_qi[i] = gen_reg_rtx (V16QImode);
>> + if (TARGET_POWERPC64)
>> + emit_insn (gen_p8_mtvsrd_v16qidi2 (vr_qi[i], op[i]));
>> + else
>> + emit_insn (gen_p8_mtvsrwz_v16qisi2 (vr_qi[i], op[i]));
>> + }
>
> TARGET_64BIT here as well.
>
> TARGET_POWERPC64 means the current machine has the 64-bit insns. It
> does not mean the code will run in 64-bit mode (e.g. -m32 -mpowerpc64 is
> just fine, and can be useful), but it also does not mean the OS (libc,
> kernel, etc.) will actually save the full 64-bit registers -- making it
> only useful on Darwin currently.
>
> (You *can* run all of the testsuite flawlessly on Linux with those
> options, but that only works because those are small, short-running
> programs. More "real", bigger and more complex programs fail in strange
> and exciting ways!)
Fixed as well. Thanks for the detailed explanation!
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr96933-1.c
>> @@ -0,0 +1,14 @@
>> +/* { dg-do compile { target { lp64 && has_arch_pwr9 } } } */
>> +/* { dg-require-effective-target powerpc_p9vector_ok } */
>> +/* { dg-options "-O2" } */
>
> As David said:
>
> /* { dg-do compile } */
> /* { dg-require-effective-target lp64 } */
> /* { dg-require-effective-target has_arch_pwr9 } */
> /* { dg-require-effective-target powerpc_p9vector_ok } */
> /* { dg-options "-O2 -mdejagnu-cpu=power9" } */
>
Updated. But I guessed the reason why we recommend to use single
line effective-target: more clear for people to read? easier to
find the check result in test debugging verbose dump content?
Anything else I missed?
> But, you probably don't want that has_arch_pwr9 line at all, this is a
> compile test?
Yeah, removed.
>
> So, okay for trunk with those TARGET_POWERPC64 fixed, and that one
> remaining testcase. Thanks!
Thanks! Bootstrapped/regress-tested on powerpc64le-linux-gnu P8/P9
and powerpc64-linux-gnu P8 again and committed in r11-4731.
BR,
Kewen
