[PATCH] D37018: [GISel]: Legalize G_PHIs

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar marked 2 inline comments as done.
aditya_nandakumar added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:687
+    auto *MBB = MI.getParent();
+    MIRBuilder.setInsertPt(*MBB, MBB->getFirstNonPHI());
+    MIRBuilder.buildTrunc(MI.getOperand(0).getReg(),
----------------
qcolombet wrote:
> Haha, nice catch!
> 
> How did this work previously?!
> Wouldn't we insert the G_TRUNC before the G_PHI? 
Previously I only had one G_PHI at a time in a test. If we setInsertPt to MI (the G_PHI being legalized) and then
Builder.buildInstr(G_PHI) + arguments
Builder.buildInstr(G_TRUNC),
we would have been fine as G_PHI is still the first instruction in the BB.

With 2 G_PHIs, we would end up with
G_PHI(s16)
G_TRUNC
G_PHI(s16)
G_TRUNC
Which is obviously wrong - so the extra test case exposed this bug :)


================
Comment at: test/CodeGen/AArch64/GlobalISel/legalize-phi.mir:122
+  }
+
+...
----------------
qcolombet wrote:
> Is there any interest in keeping the IR around?
> IMHO, there is not, so I would just go with `void funcN(void) { ret void }`. (You'll need to remove the references to the IR label from the MI BBs.)
My only use of the IR is that I find it easier to read LLVM-IR than MIR to see what the function is doing. That said, in this test (with a test description), we shouldn't need it. I'll clean this up before pushing.


https://reviews.llvm.org/D37018