[PATCH] D82262: [RISCV] Optimize addition with an immediate
Luís Marques via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 23 13:26:50 PDT 2020
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
More information about the llvm-commits
mailing list