This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: peephole or expand optimization for fixing poor code generated in conversion from QImode to PSImode pointer?


On Mon, 23 Sep 2019 10:56:55 -0500
Segher Boessenkool <segher@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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]