[PATCH] D114856: [RISCV] Override TargetLowering::BuildSDIVPow2 to generate SELECT

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:10096
+  // only for Zbt.
+  if (!Subtarget.hasStdExtZbt())
+    return SDValue();
----------------
Should say *why*, ie need conditional move


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:10100
+  AttributeList Attr = DAG.getMachineFunction().getFunction().getAttributes();
+  if (isIntDivCheap(N->getValueType(0), Attr))
+    return SDValue(N, 0); // Lower SDIV as SDIV
----------------
X86 prioritises isIntDivCheap over a lack of conditional move; what's the justification for doing it the other way round?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:10106
+  if ((VT != MVT::i32 && VT != MVT::i64) ||
+      !(Divisor.isPowerOf2() || (-Divisor).isPowerOf2()))
+    return SDValue();
----------------
craig.topper wrote:
> Use APInt::isNegatedPowerOf2
X86 asserts it's one of the two, so this check seems bogus given the function name...


================
Comment at: llvm/test/CodeGen/RISCV/rv32zbt-div-pow2.ll:7
+
+define i32 @sdiv_pow2(i32 %a) {
+; RV32I-LABEL: sdiv_pow2:
----------------
No positive power of 2 tests, and no tests for isIntDivCheap being true?


================
Comment at: llvm/test/CodeGen/RISCV/rv64zbt-div-pow2.ll:8
+define i32 @sdiv_pow2_32(i32 %a) {
+; RV64I-LABEL: sdiv_pow2_32:
+; RV64I:       # %bb.0: # %entry
----------------
Don't duplicate tests, just add rv64 check lines to your existing rv32-only test


================
Comment at: llvm/test/CodeGen/RISCV/rv64zbt-div-pow2.ll:31
+
+define i64 @sdiv_pow2_64(i64 %a) {
+; RV64I-LABEL: sdiv_pow2_64:
----------------
And we should test we don't do stupid things for i64 on RV32


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114856