On Mon, 23 Sep 2019 10:56:55 -0500 Segher Boessenkool <seg...@kernel.crashing.org> wrote:
> On Mon, Sep 23, 2019 at 12:56:20PM +0100, Jozef Lawrynowicz wrote: > > (insn 2 4 3 2 (set (reg/v:HI 25 [ i ]) > > (zero_extend:HI (reg:QI 12 R12 [ i ]))) > > (nil)) > > ..... > > (insn 7 6 8 2 (set (reg:PSI 28) > > (subreg:PSI (sign_extend:SI (reg/v:HI 25 [ i ])) 0)) > > (nil)) > > > > All we really need is: > > > > (insn (set (reg:PSI 28 [ i ]) > > (zero_extend:PSI (reg:QI 12 R12 [ i ]))) > > (nil)) > > > > The zero extend is implicit with byte sized register operations, so this > > would > > result in assembly such as: > > MOV.B R12, R12 > > > > My question is whether this is the type of thing that should be handled > > with a > > peephole optimization or if it is worth trying to fix the initial RTL > > generated > > by expand (or in a later RTL pass e.g. combine)? > > combine does (well, it did in June, I don't think things changed since then) > > Trying 2 -> 7: > 2: r25:HI=zero_extend(R12:QI) > REG_DEAD R12:QI > 7: r28:PSI=sign_extend(r25:HI)#0 > REG_DEAD r25:HI > Failed to match this instruction: > (set (reg:PSI 28 [ i ]) > (sign_extend:PSI (zero_extend:HI (reg:QI 12 R12 [ i ])))) > Failed to match this instruction: > (set (reg:PSI 28 [ i ]) > (sign_extend:PSI (and:HI (reg:HI 12 R12) > (const_int 255 [0xff])))) > > It could have just done that as > > (set (reg:PSI 28) > (zero_extend:PSI (reg:QI 12))) > > as far as I can see? Do you already have a machine pattern that matches > that? Yes that combination is what I was expecting to see. I guess since char is unsigned, a zero extend is needed to widen it, and then for the following insn a sign extend is added to widen the HImode integer. There isn't currently a pattern which matches that, but adding it doesn't fix the issue which is why I thought it might need to be fixed earlier in RTL generation. Here is the pattern that is missing: +(define_insn "movqipsi" + [(set (match_operand:PSI 0 "register_operand" "=r,r") + (zero_extend:PSI (match_operand:QI 1 "general_operand" "rYs,m")))] + "msp430x" + "@ + MOV.B\t%1, %0 + MOV%X1.B\t%1, %0" +) + So will combine potentially be able to do away with (sign_extend (zero_extend)) in certain situations? I filed PR/91865 which has all the relevant details from this thread. I can add the following nameless insn and combine will catch this case and generate the better, smaller code: +(define_insn "*movqihipsi" + [(set (match_operand:PSI 0 "register_operand" "=r,r") + (sign_extend:PSI (zero_extend:HI (match_operand:QI 1 "general_operand" "rYs,m"))))] + "msp430x" + "@ + MOV.B\t%1, %0 + MOV%X1.B\t%1, %0" +) + Thanks, Jozef > > Please file a PR for this. Thanks! > > > Segher
