Re: Fix PR37916 (unnecessary spilling)

[Jeff Law <law at redhat dot com>]

On 11/21/18 7:13 AM, Richard Biener wrote:
> On Wed, Nov 21, 2018 at 1:12 AM Jeff Law <law@redhat.com> wrote:
>>
>> On 11/20/18 6:42 AM, Michael Matz wrote:
>>> Hi,
>>>
>>> this bug report is about cris generating worse code since tree-ssa.  The
>>> effect is also visible on x86-64.  The symptom is that the work horse of
>>> adler32.c (from zlib) needs spills in the inner loop, while gcc 3 did not,
>>> and those spills go away with -fno-tree-reassoc.
>>>
>>> The underlying reason for the spills is register pressure, which could
>>> either be rectified by the pressure aware scheduling (which cris doesn't
>>> have), or by simply not generating high pressure code to begin with.  In
>>> this case it's TER which ultimately causes the register pressure to
>>> increase, and there are many plans in people minds how to fix this (make
>>> TER regpressure aware, do some regpressure scheduling on gimple, or even
>>> more ambitious things), but this patch doesn't tackle this.  Instead it
>>> makes reassoc not generate the situation which causes TER to run wild.
>>>
>>> TER increasing register pressure is a long standing problem and so it has
>>> some heuristics to avoid that.  One wobbly heuristic is that it doesn't
>>> TER expressions together that have the same base variable as their LHSs.
>>> But reassoc generates only anonymous SSA names for its temporary
>>> subexpressions, so that TER heuristic doesn't apply.  In this testcase
>>> it's even the case that reassoc doesn't really change much code (one
>>> addition moves from the end to the beginning of the inner loop), so that
>>> whole rewriting is even pointless.
>>>
>>> In any case, let's use copy_ssa_name instead of make_ssa_name, when we
>>> have an obvious LHS; that leads to TER making the same decisions with and
>>> without -fno-tree-reassoc, leading to the same register pressure and no
>>> spills.
> 
> I don't think this is OK.  Take one example, in rewrite_expr_tree for the final
> recursion case we replace
> 
>    a_1 = _2 + _3;
> 
> with sth else, like
> 
>     _4 = _5 + 1;
> 
> so we compute a new value that may not have been computed before and
> certainly not into the user variable a.  If you change this to instead create
> 
>   a_4 = _5 + 1;
> 
> then this leads to wrong debug info showing a value for 'a' that never existed.
> You can observe this with
But isn't the point to use an underlying SSA_NAME_VAR when we have one
that should be appropriate?  Are we just being too aggressive with using
copy_ssa_name?

jeff