[PATCH] D47857: [RISCV] Add machine function pass to merge base + offset

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks, I'll have to finish stepping through the code tomorrow but this seems to be working great. For the GCC torture suite at https://reviews.llvm.org/owners/package/1/, ~40 of the test cases see improved codegen (fewer instructions). Two gain a single instruction, but that's just noise due to slight regalloc differences.

One change it would be worth noting in the commit message / patch description: this pass won't run for OptNone compilation, so there will be a negative impact overall vs the old approach at O0. Not a problem - having this skipped at O0 makes sense, but worth mentioning all the same. Arguably the RISCVISelDAGToDAG peepholes should be skipped for OptNone too.



================
Comment at: lib/Target/RISCV/RISCVMergeBaseOffset.cpp:1
+//== RISCVMergeBaseOffset.cpp - Fold offset into address lowering sequence ==//
+//                     The LLVM Compiler Infrastructure
----------------
This line is only 79 chars long. Probably nicer to keep the usual format (three =) and have a shorter description.

e.g. `//== RISCVMergeBaseOffset.cpp - Fold offset into address lowering sequence ==//`


================
Comment at: lib/Target/RISCV/RISCVMergeBaseOffset.cpp:143
+  assert((TailAdd.getOpcode() == RISCV::ADD) && "Expected ADD instruction!");
+  for (MachineOperand &Op : TailAdd.operands()) {
+    if (Op.isDef())
----------------
Given that we know the MI is an Add, is looping over the operands the clearest way of handling this?


https://reviews.llvm.org/D47857