[PATCH] D126576: [RISCV] Add custom isel for (add X, imm) used by load/stores.

Tue May 31 09:36:03 PDT 2022

reames added a comment.

In D126576#3544185 <https://reviews.llvm.org/D126576#3544185>, @craig.topper wrote:

> In D126576#3544140 <https://reviews.llvm.org/D126576#3544140>, @reames wrote:
>
>> Just a thought on an alternate approach.  Feel free to ignore.
>>
>> Given:
>> addi a2, a2, Low12C
>> add a2, a0, a2
>>
>> Is there any reason we shouldn't canonicalize toward:
>> add a2, a0, a2
>> addi a2, a2, Low12C
>>
>> That is, try to push the add with immediate towards the users?  (Assume a one use restriction on the original addi.)
>>
>> This isn't an optimization per se, but if we could treat it as a canonicalization, I think it simplifies the address matching problem significantly.
>
> Where do you propose to canonicalize it? Another peephole between isel and doPeepholeLoadStoreADDI?

It turns out I had a wrong mental model on SDAG handling of constants.  I had been thinking we'd expanded the constant materialization sequences during legalization; looking at an example with debug output, this turns out not to be the case.  As such, my assumption that this was just a DAGCombine rule is off base.

Given that, yeah, the structure would seem to require a separate peephole between ISEL and the current address matching.  ("between" might be slightly the wrong word here as we could probably fixed point the later two.)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126576/new/

https://reviews.llvm.org/D126576