Re: [PATCH 0/2] RISC-V: New pass to optimize calculation of offsets for memory operations.

[Jeff Law <jeffreyalaw@gmail.com>]

On 6/15/23 09:30, Manolis Tsamis wrote:
> On Thu, Jun 15, 2023 at 6:04 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>
>>
>>
>> On 5/25/23 07:42, Jeff Law wrote:
>>
>>> Thanks Manolis.  Do you happen to know if this includes the fixes I
>>> passed along to Philipp a few months back?  My recollection is one fixed
>>> stale DF data which prevented an ICE during bootstrapping, the other
>>> needed to ignore debug insns in one or two places so that the behavior
>>> didn't change based on the existence of debug insns.
>> So we stumbled over another relatively minor issue in this code this
>> week that I'm sure you'll want to fix for a V2.
>>
>> Specifically fold_offset's "scale" argument needs to be a HOST_WIDE_INT
>> rather than an "int".  Inside the ASHIFT handling you need to change the
>> type of shift_scale to a HOST_WIDE_INT as well and potentially the
>> actual computation of shift_scale.
>>
>> The problem is if you have a compile-time constant address on rv64, it
>> might be constructed with code like this:
>>
>>
>>
>>
>>> (insn 282 47 283 6 (set (reg:DI 14 a4 [267])
>>>          (const_int 348160 [0x55000])) "test_dbmd_pucinterruptenable_rw.c":18:31 179 {*movdi_64bit}
>>>       (nil))
>>> (insn 283 282 284 6 (set (reg:DI 14 a4 [267])
>>>          (plus:DI (reg:DI 14 a4 [267])
>>>              (const_int 1365 [0x555]))) "test_dbmd_pucinterruptenable_rw.c":18:31 5 {riscv_adddi3}
>>>       (expr_list:REG_EQUAL (const_int 349525 [0x55555])
>>>          (nil)))
>>> (insn 284 283 285 6 (set (reg:DI 13 a3 [268])
>>>          (const_int 1431662592 [0x55557000])) "test_dbmd_pucinterruptenable_rw.c":18:31 179 {*movdi_64bit}
>>>       (nil))
>>> (insn 285 284 215 6 (set (reg:DI 13 a3 [268])
>>>          (plus:DI (reg:DI 13 a3 [268])
>>>              (const_int 4 [0x4]))) "test_dbmd_pucinterruptenable_rw.c":18:31 5 {riscv_adddi3}
>>>       (expr_list:REG_EQUAL (const_int 1431662596 [0x55557004])
>>>          (nil)))
>>> (insn 215 285 216 6 (set (reg:DI 14 a4 [271])
>>>          (ashift:DI (reg:DI 14 a4 [267])
>>>              (const_int 32 [0x20]))) "test_dbmd_pucinterruptenable_rw.c":18:31 204 {ashldi3}
>>>       (nil))
>>> (insn 216 215 42 6 (set (reg/f:DI 14 a4 [166])
>>>          (plus:DI (reg:DI 14 a4 [271])
>>>              (reg:DI 13 a3 [268]))) "test_dbmd_pucinterruptenable_rw.c":18:31 5 {riscv_adddi3}
>>>       (expr_list:REG_DEAD (reg:DI 13 a3 [268])
>>>          (expr_list:REG_EQUIV (const_int 1501199875796996 [0x5555555557004])
>>>              (nil))))
>>
>>
>>
>> Note that 32bit ASHIFT in insn 215.  If you're doing that computation in
>> a 32bit integer type, then it's going to shift off the end of the type.
>>
> Thanks for reporting. I also noticed this while reworking the
> implementation for v2 and I have fixed it among other things.
> 
> But I'm still wondering about the type of the offset folding
> calculation and whether it could overflow in a bad way:
> Could there also be edge cases where HOST_WIDE_INT would be problematic as well?
> Maybe unsigned HOST_WIDE_INT is more correct (due to potential overflow issues)?
I think HOST_WIDE_INT is going to be OK.  If we overflow a H_W_I, then 
there's bigger problems elsewhere.

jeff