[PATCH] D37018: [GISel]: Legalize G_PHIs

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

LGTM.

Few more nitpicks, no need for more reviews.



================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:687
+    auto *MBB = MI.getParent();
+    MIRBuilder.setInsertPt(*MBB, MBB->getFirstNonPHI());
+    MIRBuilder.buildTrunc(MI.getOperand(0).getReg(),
----------------
Haha, nice catch!

How did this work previously?!
Wouldn't we insert the G_TRUNC before the G_PHI? 


================
Comment at: test/CodeGen/AArch64/GlobalISel/legalize-phi.mir:122
+  }
+
+...
----------------
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.)


================
Comment at: test/CodeGen/AArch64/GlobalISel/legalize-phi.mir:336
+   ; CHECK-LABEL: bb.1.loop_body:
+   ; CHECK: [[RES_PHI:%.*]](s16) = G_PHI [[RES_BB1]](s16), %bb.0.entry, [[RES_BB2:%.*]](s16), %bb.1.loop_body
+   ; CHECK: [[RES_BB2]](s16) = G_ANYEXT
----------------
Could you check that we insert the G_TRUNC at the right place?


================
Comment at: test/CodeGen/AArch64/GlobalISel/legalize-phi.mir:403
+   ; CHECK-NEXT: [[RES_PHI:%.*]](s16) = G_PHI [[RES2_BB1]](s16), %bb.1.case1, [[RES2_BB2]](s16), %bb.2.case2
+
+    %0(s32) = COPY %w0
----------------
Ditto.


https://reviews.llvm.org/D37018