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