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

[Jeff Law <jeffreyalaw@gmail.com>]

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))
> (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