Re: [PR80693] drop value of parallel SETs dropped by combine

[Segher Boessenkool <segher at kernel dot crashing dot org>]

Hi!

On Thu, Dec 07, 2017 at 07:01:03PM -0200, Alexandre Oliva wrote:

[ Big snip; thanks for all the detailed information! ]

> Still, I'm concerned I haven't caught all of the cases in which
> adjusting REG_N_SETS would be needed: we might have dropped multiple
> SETs of the same pseudo, each with its own REG_UNUSED note (say, from
> all of i3, i2, i1, and i0), and the current logic will only decrement
> REG_N_SETS once, and only if i2 no longer sets the register.

The only thing that matters for combine is that if REG_N_SETS == 1, then
it must be correct.  So "forgetting" to decrement is okay, forgetting
to increment is not.

> > It wasn't obvious to me (this code is horribly complicated).  Whether
> > all existing code is correct...  it's probably best not to look too
> > closely :-/
> 
> You're right about its being horribly complicated.

I would like to remove all regstat things from combine, use "real" DF
instead.  This will simplify many things, and allow better optimisation
as well.  But is it fast enough?  We'll see.

> > If you have a patch you feel confident in, could you post it again
> > please?
> 
> So let me tell you how I feel about this.  It has waited long enough,
> and there are at least 3 bugs known to be fixed by the first very simple
> patch below.  The catch is that it doesn't adjust REG_N_SETS at all (we
> didn't before the patch, and that didn't seem to hurt too much).  I've
> regstrapped this successfully on x86_64-linux-gnu and i686-linux-gnu.


> 	PR rtl-optimization/80693
> 	PR rtl-optimization/81019
> 	PR rtl-optimization/81020
> 	* combine.c (distribute_notes): Reset any REG_UNUSED REGs that
> 	are not mentioned in i3.  Place the REG_UNUSED note on i2,
> 	possibly modified to REG_DEAD, if it did not originate in i3.
> 
> for  gcc/testsuite/ChangeLog
> 
> 	PR rtl-optimization/80693
> 	PR rtl-optimization/81019
> 	PR rtl-optimization/81020
> 	* gcc.dg/pr80693.c: New.
> 	* gcc.dg/pr81019.c: New.


> But then it occurred to me that the REG_UNUSED register from i1 might
> have *also* been set in i2, and we might then have used it as a scratch
> register for split, while the set that the REG_UNUSED pertained to might
> have been completely discarded from a parallel in i1 or i0.  And then I
> wasn't unsure whether or not to decrement REG_N_SETS.

[ more snip ]

> Then I realized we might have to also reset the cached value when it's
> referenced in i3, and possibly also adjust the counts.  And also some of
> that when it's set in i3.  Then I lost it.  Help? :-)
> 
> FWIW, I'd be glad just installing the patch between --->cut<---s, or
> even a simpler version thereof that just resets the recorded value and
> doesn't even attempt to place notes in i2, to get the bugs fixed while
> we sort out the really tricky issues of adjusting REG_N_SETS.  The
> (incomplete, but functional) fix has been known for a while, and we
> shouldn't subject users to wrong code when we can help it, even if we
> might miss optimization opportunities for that, right?  Thoughts?

Yes, that first patch is okay for trunk.  Thanks for all the work on this!

I don't think this patch makes anything worse, and it does make some things
better.


Segher