[PATCH] D62857: [RISCV] Prevent re-ordering some adds after shifts

[Sam Elliott via Phabricator via llvm-commits]

lenary marked 3 inline comments as done.
lenary added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:869
+
+        if (C1Seq.size() <= ShiftedC1Seq.size())
+          return false;
----------------
lewis-revill wrote:
> Isn't there an inaccuracy in this method of checking the materialization cost since `RISCVMatInt::generateInstSeq` always calculates the cost of materializing into a register? In this case we have instructions which might use immediates, but this will calculate the cost as being the same as a single-instruction materialization into a register followed by an instruction using a register.
> 
> IE:
> 
> ```
> addi rd, rs1, C
> ```
> 
> would appear to be the same cost as:
> 
> ```
> lui rs2, (C >> 12)
> add rd, rs1, rs2
> ```
> 
On line 860, we check that `C1` (which you're calling `C`) will fit into an add immediate. If it will, that counts as "free", and we definitely want to prevent the re-ordering, and we don't even check the materialisation cost.

In fact, I'm going to update the patch to always allow the re-ordering if `C1 << C2` will fit into an add immediate, because then we also know that materialisation of that constant is "free", and so we should allow the re-ordering because it might help later dagcombines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62857