Re: RTL CSE picking simpler but more expensive expressions (re: PR 65932)

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

Hi Jeff,

On 13/01/16 19:28, Jeff Law wrote:
> On 01/13/2016 04:33 AM, Kyrill Tkachov wrote:
> > I've been able to get it to do the right thing by changing the line
> > where it initially folds the source of the SET. That is line 4639 in
> > cse.c: /* Simplify and foldable subexpressions in SRC.  Then get the
> > fully- simplified result, which may not necessarily be valid. */
> > src_folded = fold_rtx (src, insn);
> >
> > In this instance SRC is: (plus:SI (mult:SI (sign_extend:SI (subreg:HI
> > (reg:SI 136) 0)) (sign_extend:SI (subreg:HI (reg:SI 138) 0))) (reg:SI
> > 141))
> >
> > and the resulting src_folded is: (plus:SI (mult:SI (reg:SI 136)
> > (reg:SI 138)) (reg:SI 141))
> >
> > However, fold_rtx also modifies src itself, so after the call to
> > fold_rtx both src and src_folded contain the plus+mult form without
> > the extends. So further down in cse_insn where it does the cost
> > analysis of src, src_folded and other expressions it never considers
> > the original form of src. Changing that call to fold_rtx to not
> > modify its argument like so: src_folded = fold_rtx (src, 0); // An
> > argument of 0 means "make a copy of src before modifying"
> >
> > fixes the testcase and allows CSE to properly select the cheaper
> > multiply-extend-add form and doesn't seem to regress codegen in any
> > way on SPEC2006 for arm. Archeology says this line has been that way
> > since forever, so does anyone know of the rationale for passing insn
> > to fold_rtx in that callsite?
> That callsite was added during gcc-2 development, in an era where we didn't even have public lists where such a change might have been discussed.  I didn't try to dig into the old, private gcc2 archives as it's unlikely there's going to 
> be any rationale there.
>
> ISTM the code clearly expects that SRC and SRC_FOLDED could be different.  I think you could make a case that INSN should just be replaced with NULL based on that alone.  Verifying across a reasonable body of code that there aren't any 
> undesirable effects would be wise.

There are cases where calls to fold_rtx with a non-NULL insn don't end up modifying src, ending up in src to not be
always equal to src_folded (I added an assert to that effect and saw it trigger).
It seems that fold_rtx is not *guaranteed* to modify src if insn is non-NULL, but it just does in many cases.

> Alternately you could compute stuff for "SRC" prior to the call to fold_rtx. It's less likely to have unexpected side effects.
>
> My inclination would be to go with changing INSN to NULL though. It seems to match the overall intent here better.

I agree and, as I said, I saw no impact on codegen for all of SPEC2006 on arm. It just improved the testcase
I described above. I'll evaluate this patch on aarch64 and x86_64 and hopefully it has a low impact there as well.

Thanks,
Kyrill

> Jeff