[committed][rtl-optimization/116244] Don't create bogus regs in alter_subreg

[Jeff Law]

On 2/3/25 2:09 AM, Richard Sandiford wrote:

>>>
>>> So yeah, I think the first question is why ira_build_conflicts isn't
>>> kicking in for this register or (if it is) why we still get register 0.
>> So pulling on this thread leads me into the code that sets up
>> ALLOCNO_WMODE in create_insn_allocnos:
>>
>>>            if ((a = ira_curr_regno_allocno_map[regno]) == NULL)
>>>              {
>>>                a = ira_create_allocno (regno, false, ira_curr_loop_tree_node);
>>>                if (outer != NULL && GET_CODE (outer) == SUBREG)
>>>                  {
>>>                    machine_mode wmode = GET_MODE (outer);
>>>                    if (partial_subreg_p (ALLOCNO_WMODE (a), wmode))
>>>                      ALLOCNO_WMODE (a) = wmode;
>>>                  }
>>>              }
>> Note how we only set ALLOCNO_MODE only at allocno creation, so it'll
>> work as intended if and only if the first reference is via a SUBREG.
> 
> Huh, yeah, I agree that that looks wrong.
> 
>> ISTM the fix here is to always do the check and set ALLOCNO_WMODE.
>>
>> The other bug I see is that we may potentially have paradoxicals in
>> different modes.  ie, on a 32 bit target, we could in theory have a
>> paradoxical in DI and another in TI.  So in addition to pulling that
>> code out of the conditional so that it executes every time, the
>> assignment would look like
>>
>> if (partial_subreg_p (ALLCONO_WMODE (a), wmode)
>>       && wmode > ALLOCNO_WMODE (a))
>>     ALLOCNO_WMODE (a) = wmode;
>>
>> Or something along those lines.
> 
> Not sure about this part though.  The construct:
> 
>    if (partial_subreg_p (ALLCONO_WMODE (a), wmode))
>      ALLOCNO_WMODE (a) = wmode;
> 
> is effectively:
> 
>    ALLOCNO_WMODE (a) = MAX_SIZE (ALLOCNO_WMODE (a), wmode);
You're right.

> 
> and so already picks the single widest mode, if there is one.
> For things like DI vs DF, it will use the existing mode as a tie-breaker.
> 
> So ISTM that moving the code out of the "if (... == NULL)" should be
> enough on its own.
It is and that's actually the patch that's been in my tester for the 
last week or so.  The other (non)issue wasn't necessary to fix the 
problem, just something that looked odd/wrong as I was preparing the 
email and I included it in the discussion.

Jeff