[PATCH] D82262: [RISCV] Optimize addition with an immediate

[Luís Marques via Phabricator via llvm-commits]

luismarques added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:85
+        break;
+      // The immediate must be in range [-4095,-2049] or [2048,4094].
+      int64_t ImmVal = ConstOp->getSExtValue();
----------------
Given that you can have an ADDI of `-2048`, that should be `-4096`, not `-4095`. (Please update the comment, the code and the test results accordingly).


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:87-88
+      int64_t ImmVal = ConstOp->getSExtValue();
+      if (ImmVal < -4095 || (-2049 < ImmVal && ImmVal < 2048) || 4094 < ImmVal)
+        break;
+      // Break the immediate to Imm0+Imm1.
----------------
This is a matter of style so it's not very important, but I would lean towards implementing this check with a comparison logic that matches the comment about the required range, as that would be make it easier to check at a glance.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:92-93
+      EVT VT = Node->getValueType(0);
+      SDValue ImmOp0 = CurDAG->getTargetConstant(ImmVal / 2, DL, VT);
+      SDValue ImmOp1 = CurDAG->getTargetConstant(ImmVal - ImmVal / 2, DL, VT);
+      auto *NodeAddi0 = CurDAG->getMachineNode(RISCV::ADDI, DL, VT,
----------------
Nitpick: make it the other way around, so that the second addi is the "smaller" one. That generated code seems more intuitive to me that way :-)


================
Comment at: llvm/test/CodeGen/RISCV/add-imm.ll:9-25
+define i32 @add32_0(i32 %a) nounwind {
+; RV32I-LABEL: add32_0:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    addi a0, a0, 2047
+; RV32I-NEXT:    ret
+  %1 = add i32 %a, 2047
+  ret i32 %1
----------------
When you have `i32` and the `i64` tests they should both have riscv32 and riscv64 test CHECKs. You can have operations on `i64`s in riscv32, they just doesn't correspond to a single native instruction. But in this case I don't see much advantage in having test variants for both types, as this optimization should be orthogonal to that. I suggest keeping one `i32`+`i64` test (to show that it works with both types), and trimming the remaining tests down to just the `i32` variant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82262