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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
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
----------------
jrtc27 wrote:
> X86 prioritises isIntDivCheap over a lack of conditional move; what's the justification for doing it the other way round?
I agree with @jrtc27. The isIntDivCheap check should be at the top.


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


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

We should also add a test for dividing by 4096 or larger for RV32 and RV64. And probably something larger than 2^32 for RV64. The constant materialization will add additional instructions not seen in the 1024 case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114856