Re: [PATCH v2] Store_bit_field_1: Use SUBREG instead of REG if possible

[Richard Sandiford <richard.sandiford@arm.com>]

Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On 7/19/23 04:25, Richard Biener wrote:
>> On Wed, 19 Jul 2023, YunQiang Su wrote:
>> 
>>> Eric Botcazou <botcazou@adacore.com> ?2023?7?19??? 17:45???
>>>>
>>>>> I don't see that.  That's definitely not what GCC expects here,
>>>>> the left-most word of the doubleword should be unchanged.
>>>>>
>>>>> Your testcase should be a dg-do-run and probably more like
>>>>>
>>>>> NOMIPS16 int __attribute__((noipa)) test (const unsigned char *buf)
>>>>> {
>>>>>    int val;
>>>>>    ((unsigned char*)&val)[0] = *buf++;
>>>>>    ((unsigned char*)&val)[1] = *buf++;
>>>>>    ((unsigned char*)&val)[2] = *buf++;
>>>>>    ((unsigned char*)&val)[3] = *buf++;
>>>>>    return val;
>>>>> }
>>>>> int main()
>>>>> {
>>>>>    int val = 0x01020304;
>>>>>    val = test (&val);
>>>>>    if (val != 0x01020304)
>>>>>      abort ();
>>>>> }
>>>>>
>>>>> not sure if I got endianess correct.  Now, the question is what
>>>>> WORD_REGISTER_OPERATIONS implies for a bitfield insert and what
>>>>> the MIPS ABI says for returning SImode.
>>>>
>>>
>>> MIPS N64 ABI uses 2 GPR for integer return values.
>>> If the return value is SImode, the first v0 register is used, and it
>>> must be sign-extended,
>>> aka the bits[64-31] are all same.
>>>
>>> Yes, it is same for signed and unsigned int32.
>>>
>>> https://irix7.com/techpubs/007-2816-004.pdf
>>> Page 6:
>>> 32-bit integer (int) parameters are always sign-extended when passed
>>> in registers,
>>> whether of signed or unsigned type. [This issue does not arise in the
>>> o32-bit ABI.]
>> 
>> Note I think Andrews comment#7 in the PR is spot-on then, the issue
>> isn't the bitfield inserts but the compare where combine elides
>> the sign_extend in favor of a subreg.  That's likely some wrongdoing
>> in simplify-rtx in the context of WORD_REGISTER_OPERATIONS.
> And I think it raises a real question about the use of GPR (which maps 
> to SImode and DImode for 64bit MIPS targets) on the conditional 
> branching patterns in mips.md.
>
> So while this code works:
>
>> (insn 20 19 23 2 (set (reg/v:DI 200 [ val+-4 ])
>>         (sign_extend:DI (subreg:SI (reg/v:DI 200 [ val+-4 ]) 4))) "/app/example.cpp":7:29 -1
>>      (nil))

Haven't had chance to compile and look at it properly, but this subreg
seems suspicious for MIPS, given the definition of TRULY_NOOP_TRUNCATION.
We should instead use a truncdisi2 to narrow reg:DI 200 to an SI register,
and then sign_extend it.

This is easily missed in target-independent code because so few targets
define TRULY_NOOP_TRUNCATION.

Where is the subreg being generated?

Richard

>> (jump_insn 23 20 24 2 (set (pc)
>>         (if_then_else (le (subreg/s/u:SI (reg/v:DI 200 [ val+-4 ]) 4)
>>                 (const_int 0 [0]))
>>             (label_ref 32)
>>             (pc))) "/app/example.cpp":8:5 -1
>>      (int_list:REG_BR_PROB 440234148 (nil))
>>  -> 32)
>
>
> Normally the narrowing SUBREG in insn 23 would indicate we don't care 
> about the bits outside SImode.  But on a W_R_O targets we very much care 
> because the hardware is going to ultimately do the comparison in 64 bits.
>
> As Andrew/Richi have indicated this very much points to combine as 
> incorrectly eliminating the explict sign extension.  Most likely because 
> something saw the SUBREG and concluded those upper bits set by insn 20 
> were "don't care" bits.
>
> But it may ultimately be be better for the MIPS port to not expose a 
> SImode comparison.  Thus reducing the reliance on W_R_O and its 
> under-specified semantics and ultimately having the RTL map more closely 
> to what the hardware actually does/supports.
>
> That's the model we're working towards on the RISC-V port as well.  I 
> wouldn't be surprised if we eventually get to the point where we 
> eliminate WORD_REGISTER_OPERATIONS entirely.
>
> And yes, bitfield operations are one of the nasty sticking points.  The 
> thinking for them is that we want to support bit manipulations where the 
> bit position is variable.  To do that we will emit an explicit sign 
> extension after such operations.  Then rely on improved REE to identify 
> and remove those redundant extensions.
>
> Jeff
>
> Jeff