[PATCH] D72833: [GlobalISel] Use more MachineIRBuilder helper methods

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1989
+    Observer.changingInstr(MI);
+    auto &TII = *MI.getMF()->getSubtarget().getInstrInfo();
+    MI.setDesc(TII.get(TargetOpcode::G_MUL));
----------------
MIRBuilder.getII()


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1995
+    MIRBuilder.setInsertPt(MIRBuilder.getMBB(), ++MIRBuilder.getInsertPt());
+
     Register HiPart = MRI.createGenericVirtualRegister(Ty);
----------------
foad wrote:
> In case you were wondering about this hunk: the old code left the G_SMULO/UMULO in place, which defined Res, and also created a new G_MUL, which defined Res. This caused an assertion failure when buildAShr tried to constant fold its arguments, because it wasn't expecting to find more than one definition of Res. Is this a general problem in GlobalISel lowering and is my workaround reasonable?
It looks to me like the old code didn't modify the instruction in place, the new code does. Most places create new defs of the original result before erasing the original instruction, so this might be a widespread issue with the CSEMIRBuilder


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72833