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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I'm a bit confused, where in the code does this actually create functions? It doesn't look like expandDivision() does that, and I can't spot anything else creating `__llvm_udivXXX` either.



================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:40
+
+  for (auto &I : make_early_inc_range(instructions(F))) {
+
----------------
You should either do the replacement directly in this loop, or not use make_early_inc_range. Doesn't make sense to use it if you're not modifying anything.


================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:41
+  for (auto &I : make_early_inc_range(instructions(F))) {
+
+    switch (I.getOpcode()) {
----------------
Unnecessary newline


================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:68
+        I->getOpcode() == Instruction::SDiv) {
+      expandDivision(cast<BinaryOperator>(I));
+    } else {
----------------
Could use a vector of BinaryOperator to avoid the casts here.


================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:84
+
+  return PreservedAnalyses::all();
+}
----------------
At least for the new pass manager, I don't think it's legal to add new functions in a function pass. I'm not sure about the legacy pass manager.


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:859
 
+
   if (getOptLevel() != CodeGenOpt::None) {
----------------
Spurious change.


================
Comment at: llvm/lib/Transforms/Utils/IntegerDivision.cpp:35
   unsigned BitWidth = Dividend->getType()->getIntegerBitWidth();
-  ConstantInt *Shift;
-
-  if (BitWidth == 64) {
-    Shift = Builder.getInt64(63);
-  } else {
-    assert(BitWidth == 32 && "Unexpected bit width");
-    Shift = Builder.getInt32(31);
-  }
+  ConstantInt *Shift = Builder.getIntN(BitWidth, BitWidth - 1);
 
----------------
Precommit as NFC changes?


================
Comment at: llvm/test/CodeGen/X86/urem-seteq.ll:380
-; X64-NEXT:    sete (%rax)
-; X64-NEXT:    retq
   %L10 = load i448, i448* undef, align 4
----------------
What happened here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126644