[PATCH] D110890: [GlobalISel] Port the udiv -> mul by constant combine.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:603
+  MachineInstr *buildUDivUsingMul(MachineInstr &MI);
+  bool matchUDivByConst(MachineInstr &MI);
+  void applyUDivByConst(MachineInstr &MI);
----------------
could use a doxygen comment?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4409
 
+/// Given an G_UDIV \p MI expressing a divide by constant, return an expression
+/// that implements it by multiplying by a magic number.
----------------
doxygen comment should be in the header, right?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4476
+  (void)Matched;
+  assert(Matched);
+
----------------
might as well have a message here


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4486
+  } else {
+    assert(MRI.getType(RHS).isScalar());
+    PreShift = PreShifts[0];
----------------
might as well have a message here too


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4513
+  auto One = MIB.buildConstant(Ty, 1);
+  auto IsOne = MIB.buildICmp(
+      CmpInst::Predicate::ICMP_EQ,
----------------
Should there be a legalizer check here? I think this will only work pre-legalize.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4536
+
+  if (MF.getFunction().hasMinSize())
+    return false;
----------------
comment explaining why we only do this for size?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110890