Re: [PATCH 2/2] rtl-optimization/113255 - avoid re-associating REG_POINTER MINUS

[Jeff Law <jeffreyalaw@gmail.com>]

On 2/5/24 01:15, Richard Biener wrote:

>>>
>>>   PR rtl-optimization/113255
>>>   * simplify-rtx.cc (simplify_context::simplify_binary_operation_1):
>>>   Do not re-associate a MINUS with a REG_POINTER op0.
>> Nasty little set of problems.  I don't think we ever pondered that we could
>> have multiple REGNO_POINTER_FLAG objects in the same expression, but clearly
>> that can happen once you introduce a 3rd term in the expression.
>>
>> I don't mind avoiding the reassociation, but it feels like we're papering over
>> problems in alias.cc.  Conceptually it seems like if we have two objects with
>> REG_POINTER set, then we can't know which one is the real base.  So your patch
>> in the PR wasn't that bad.
> 
> It wasn't bad, it's the only correct fix.  The question is what we do
> for branches (or whether we do anything there) and whether we just accept
> that that fix causes some optimization regressions.
For the branches, I'd go whatever you feel the safest change is.  While 
it looks like some of this is fundamentally broken, it can't be *that* 
bad since it's just rearing its ugly head now.

I could even make a case that going with the patch from the PR for the 
branches is reasonable.  It's attacking at least part of the root problem.

> 
>> Alternately, just stop using REG_POINTER for alias analysis?   It looks
>> fundamentally flawed to me in that context.  In fact, one might argue that the
>> only legitimate use would be to indicate to the target that we know a pointer
>> points into an object.  Some targets (the PA) need this because x + y is not
>> the same as y + x when used as a memory address.
>>
>> If we wanted to be a bit more surgical, drop REG_POINTER from just the MINUS
>> handling in alias.cc?
> 
> The problem is that REG_POINTER is just used as a heuristic
> (and compile-time optimization) as to which of a binary operator
> operands we use a base of (preferrably).  find_base_{term,value}
> happily look at operands that are not REG_POINTER (that are
> not REG_P), since for the case in question, even w/o re-assoc
> there would be no way to say the inner MINUS is not a pointer
> (it's a REG flag).
> 
> The heuristics don't help much when passes like DSE use CSELIB
> and combine operations like above, we then get to see that
> the way find_base_{term,value} perform pointer analysis is
> fundamentally flawed.  Any tweaking there has the chance to
> make other cases run into wrong base discoveries.
> 
Exactly.  So maybe I'm missing something -- it sounds like we both agree 
that using REG_POINTER in the aliasing code is just fundamentally broken 
in the modern world (and perhaps has been for a long time).  So we 
"just" need to excise that code from alias.cc.




> I'll take it that we need to live with the regressions for GCC 14
> and the wrong-code bug in GCC 13 and earlier.
I'm not sure I agree with this statement.  Or maybe I thought the patch 
in the PR was more effective than it really is.  At some level we ought 
to be able to cut out the short-cuts enabled by REG_POINTER.  That runs 
the risk of perturbing more code, but it seems to me that's a risk we 
might need to take.

jeff