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

[Manolis Tsamis <manolis.tsamis@vrull.eu>]

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

Manolis

>
> Jeff