Re: [PATCH] Optimize away x86 mem stores of what the mem contains already (PR rtl-optimization/79593)

[Uros Bizjak <ubizjak@gmail.com>]

On Tue, Jan 8, 2019 at 10:27 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Jan 08, 2019 at 08:29:03AM +0100, Uros Bizjak wrote:
> > Is there a reason stack registers are excluded? Before stackreg pass,
> > these registers are just like other hard registers.
>
> I was just afraid of those, after stack pass removing a store but not
> load would result in the stack getting out of sync, and I wasn't sure if the
> stack pass will tolerate a RA chosen stack reg not being touched anymore.

It does. There is a stack compensating code that handles these situations.

> Also, doesn't loading a float/double/long double into a stack reg and
> storing again canonicalize it in any way?  Say NaNs?  Or unnormal or
> pseudo-denormal long doubles etc.?  Tried that now with a sNaN and the
> load/store didn't change it, but haven't tried with exceptions enabled to
> see if it would raise one.

FLD from memory in SF and DFmode is considered a conversion, and
converts sNaN to NaN (and emits #IA exception). But sNaN handling is
already busted in the compiler as RA is free to spill the register in
non-XFmode. IMO, the peephole2 pattern is no worse than the current
situation.

> If that is fine, I'll leave that out.

I think we are good to do so.

> Looking around, I was using a wrong macro anyway and am surprised nothing
> complained, it should have been STACK_REG_P rather than STACK_REGNO_P.
>
> > Other that that, there is no need for REG_P predicate; after reload we
> > don't have subregs and register_operand will match only hard regs.
>
> Ok, I wasn't sure because "register_operand" allows (subreg (reg)) even if
> reload_completed, just disallows subregs of mem after that.

At least for x86, there are no SUBREGs after reload, otherwise other
parts of the compiler would break.

> > Also, please put peep2_reg_dead_p predicate in the pattern predicate.
>
> I don't see how, that would mean I'd have to write two peephole2s instead of
> one.  It tries to deal with two different cases, one is where the temporary
> reg is dead, in that case we can optimize away both the load or store, the
> second case is where the temporary reg isn't dead, in that case we can
> optimize away the store, but not the load.  With the optimizing away of both
> load and store I was just trying to do a cheap DCE there.

I didn't realize this is an optimization, a comment would be welcome here.

Uros.

> Looking around more, I actually think I need to replace
> (match_dup 1) with (match_operand 2 "memory_operand"), add rtx_equal_p
> (operands[1], operands[2]) and !MEM_VOLATILE_P (operands[2]), because
> apparently rtx_equal_p doesn't check the MEM_VOLATILE_P bit.
>
> > > 2019-01-07  Jakub Jelinek  <jakub@redhat.com>
> > >
> > >         PR rtl-optimization/79593
> > >         * config/i386/i386.md (reg = mem; mem = reg): New define_peephole2.
> > >
> > > --- gcc/config/i386/i386.md.jj  2019-01-01 12:37:31.564738571 +0100
> > > +++ gcc/config/i386/i386.md     2019-01-07 17:11:21.056392168 +0100
> > > @@ -18740,6 +18740,21 @@ (define_peephole2
> > >                        const0_rtx);
> > >  })
> > >
> > > +;; Attempt to optimize away memory stores of values the memory already
> > > +;; has.  See PR79593.
> > > +(define_peephole2
> > > +  [(set (match_operand 0 "register_operand")
> > > +        (match_operand 1 "memory_operand"))
> > > +   (set (match_dup 1) (match_dup 0))]
> > > +  "REG_P (operands[0])
> > > +   && !STACK_REGNO_P (operands[0])
> > > +   && !MEM_VOLATILE_P (operands[1])"
> > > +  [(set (match_dup 0) (match_dup 1))]
> > > +{
> > > +  if (peep2_reg_dead_p (1, operands[0]))
> > > +    DONE;
> > > +})
> > > +
> > >  ;; Attempt to always use XOR for zeroing registers (including FP modes).
> > >  (define_peephole2
> > >    [(set (match_operand 0 "general_reg_operand")
>
>         Jakub