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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

It's very unusual for a lowering strategy to introduce a new function. I would expect to expand the instruction inline and not emit a separate function, which avoids the need for this to be a module pass.



================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:53-59
+  F->addFnAttr(Attribute::NoUnwind);
+  F->addFnAttr(Attribute::WillReturn);
+  F->addFnAttr(Attribute::NoRecurse);
+  F->addFnAttr(Attribute::ReadNone);
+  // In general, div cannot be 'speculatable' due to UB when dividing by
+  // zero, but the algorithm used here doesn't produce UB.
+  F->addFnAttr(Attribute::Speculatable);
----------------
Can you set these all at once with AttributeList 


================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:218
+static bool runImpl(Function &F) {
+  SmallVector<Instruction *, 4> Replace;
+
----------------
You shouldn't need to pre-accumulate the instructions, just use make_early_inc_range on the instruction iterator?


================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:226-227
+    case Instruction::SRem:
+      if (!isa<IntegerType>(I.getType()) ||
+          I.getType()->getIntegerBitWidth() <= ExpandDivRemBits)
+        continue;
----------------
dyn_cast instead of isa + implicit cast to IntegerType?


================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:227
+      if (!isa<IntegerType>(I.getType()) ||
+          I.getType()->getIntegerBitWidth() <= ExpandDivRemBits)
+        continue;
----------------
Since this is ultimately a workaround for SelectionDAG, ideally this would be a TargetLowering control


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126644