Hi Mike, On Wed, Nov 08, 2017 at 03:02:30PM -0500, Michael Meissner wrote: > > Should this somehow be joined with p9_xxbrd_<mode>? Or maybe you should > > generate that, instead. > > No, since p9_xxbrd_<mode> doesn't include DImode. We could add yet another > mode iterator to include DI + V2DI/V2DF modes, but that seems to be overkill.
Having separate DI and V2DI patterns for the same instruction isn't great either. If it is only this insn it is fine of course, but I suspect we'll get many more later. Well I guess we can solve it later ;-) > I simplified it to only change the one insn that would normally match the > register to register case to skip if ISA 3.0. I put a test on bswapdi2_reg as > well. Thanks. > > > > @@ -2507,6 +2513,8 @@ (define_expand "bswapdi2" > > > emit_insn (gen_bswapdi2_load (dest, src)); > > > else if (MEM_P (dest)) > > > emit_insn (gen_bswapdi2_store (dest, src)); > > > + else if (TARGET_P9_VECTOR) > > > + emit_insn (gen_bswapdi2_xxbrd (dest, src)); > > > else > > > emit_insn (gen_bswapdi2_reg (dest, src)); > > > DONE; > > > > Pity that you cannot easily do similar for HI and SI. > > Not really. Bswap64 generates 9 separate insns, while bswap32 and bswap16 > only > generate 3 insns. So, having to add the move direct to/from the vector > registers would mean it would be slower than the normal case that is currently > generated. But if the value happens to be in a VSX register, then we can do > it > in one instruction. I meant, have just two lines as above :-) > After a burn-in period, I plan to backport a reduced version of the patch > (XXBRD only) to gcc 7, unless you think it shouldn't go into gcc 7. Well, why do we want it on 7? > +(define_insn "bswapdi2_xxbrd" > + [(set (match_operand:DI 0 "gpc_reg_operand" "=wo") > + (bswap:DI (match_operand:DI 1 "gpc_reg_operand" "wo")))] > + "TARGET_POWERPC64 && TARGET_P9_VECTOR" > + "xxbrd %x0,%x1" > + [(set_attr "type" "vecperm")]) This doesn't need TARGET_POWERPC64 I think. Please look at that. The patch is okay for trunk. Thanks! Segher
