[PATCH] D126644: [llvm/CodeGen] Add ExpandLargeDivRem pass

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:12
+// This is useful for backends like x86 that cannot lower divisions
+// with more than 128 bits.
+//
----------------
I think but haven't checked that 32-bit x86, ARM, RISCV32, and other 32-bit targets can't lower division with more than 64 bits.


================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:33
+    ExpandDivRemBits("expand-div-rem-bits", cl::Hidden, cl::init(128),
+                     cl::desc("div and rem instructions on integers with <N> "
+                              "or more bits are expanded."));
----------------
This says "<N> or more bits", but the code exits for `<= ExpandDivRemBits`


================
Comment at: llvm/lib/Transforms/Utils/IntegerDivision.cpp:145
 
-  ConstantInt *Zero;
-  ConstantInt *One;
-  ConstantInt *NegOne;
-  ConstantInt *MSB;
-
-  if (BitWidth == 64) {
-    Zero      = Builder.getInt64(0);
-    One       = Builder.getInt64(1);
-    NegOne    = ConstantInt::getSigned(DivTy, -1);
-    MSB       = Builder.getInt64(63);
-  } else {
-    assert(BitWidth == 32 && "Unexpected bit width");
-    Zero      = Builder.getInt32(0);
-    One       = Builder.getInt32(1);
-    NegOne    = ConstantInt::getSigned(DivTy, -1);
-    MSB       = Builder.getInt32(31);
-  }
+  ConstantInt *Zero      = Builder.getIntN(BitWidth, 0);
+  ConstantInt *One       = Builder.getIntN(BitWidth, 1);
----------------
You already have the DivTy, why not go through ConstantInt::get for all of these?


================
Comment at: llvm/test/Transforms/ExpandLargeDivRem/urem129.ll:14
+; CHECK-NEXT:    [[TMP5:%.*]] = icmp ugt i129 [[TMP4]], 128
+; CHECK-NEXT:    [[TMP6:%.*]] = or i1 [[TMP1]], [[TMP5]]
+; CHECK-NEXT:    [[TMP7:%.*]] = icmp eq i129 [[TMP4]], 128
----------------
Is this code handling the possible poison result from the ctlz's incorrectly? If [A] is zero then [TMP3] is poison, [TMP4] is poison, [TMP5] is poison, [TMP6] being an or won't block the poison. Assuming I understand poison correctly.

I don't think that was caused by this patch. It's an issue in the existing code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126644