Re: [PATCH] combine: Don't optimize SIGN_EXTEND of MEM on WORD_REGISTER_OPERATIONS targets [PR113010]

[Jeff Law <jeffreyalaw@gmail.com>]

On 2/1/24 18:24, Greg McGary wrote:
> On 1/18/24 9:24 AM, Jeff Law wrote:
>>
>> On 1/17/24 20:53, Greg McGary wrote:
>>>
>>> While the code comment is true, perhaps it obscures the primary intent,
>>> which is recognition that the pattern (SIGN_EXTEND (mem ...) ) is 
>>> destined
>>> to expand into a single memory-load instruction and no simplification is
>>> possible, so why waste time with further analysis or transformation? 
>>> There
>>> are plenty of other conditions that also short circuit to "do 
>>> nothing" and
>>> this seems just as straightforward as those others. Efforts to catch 
>>> this
>>> further downstream add gratuitous complexity.
>> Because the real bug is likely still lurking, waiting for something 
>> else to trigger it.
>>
>> An early exit is fine when we're just trying to avoid unnecessary 
>> work, but there's something else going on here we need to understand 
>> first.
> 
> 
> expand_compound_operation() wants to evaluate the sign- and 
> zero-extension of MEM. It begins by calling gen_lowpart(), which returns 
> the same result in both cases: a paradoxical subreg of a MEM (PSoM). 
> What is the value of the high part of a PSoM? Can that high part be 
> evaluated at compile time?
Potentially, yes.   If LOAD_EXTEND_OP returns ZERO_EXTEND, then we know 
at compile time those bits are zero.

I could come up with ways to optimize the SIGN_EXTEND case as well, but 
that would require some state tracking and it doesn't immediately look 
like expand_compound_operation or its children use any of the state 
tracking that's available in combine.  So for the sake of this problem, 
let's consider the SIGN_EXTEND case as not computable at compile-time in 
expand_compound_operation.



> However, for a machine where (WORD_REGISTER_OPERATIONS && load_extend_op 
> (inner_mode) == SIGN_EXTEND), the high part of a PSoM is  only known at 
> runtime as 0s or 1s. That's the downstream bug. The fix for such 
> machines is either (A) forbid static evaluation of the high part of a 
> PSoM, or (B) forbid transforming (SIGN_EXTEND (MEM ...) ) into a PSoM. 
> My patch does B. Perhaps you prefer A? The trouble with A is that in the 
> zero-extend case, it is valid to statically evaluate as 0. It is only 
> the sign-extend case that isn't known until runtime. By the time we have 
> a PSoM, its upstream ancestor as sign- or zero-extend is already lost.
> 
> Does that give you the understanding you desire, or are there deeper 
> mysteries to probe?
It's a good start and I can see what you're trying to do -- and it may 
in fact be correct -- the quick discussion with Palmer Tuesday and your 
follow-up have helped a lot).

But just to be sure, what's the incoming rtl at function entry?  just 
"debug_rtx (x)" should be sufficient.

jeff