Page MenuHomePhabricator

[AArch64][GlobalISel] Handle multiple phis in fixupPHIOpBanks
ClosedPublic

Authored by paquette on Jun 2 2021, 5:52 PM.

Details

Summary

If we ended up with two phi instructions in a block, and we needed to fix up the banks for the first one, we'd end up inserting our COPY before the second phi.

E.g.

%x = G_PHI ...
%fixup = COPY ...
%y = G_PHI ...

This is invalid MIR, and breaks assumptions made by the register allocator later down the line. With the verifier enabled, it also emits a verification error.

This teaches fixupPHIOpBanks to walk past any phi instructions in the block when emitting the fixup copies.

Here's an example of the crashing code (same as added testcase):
https://godbolt.org/z/h5j1x3o6e

Diff Detail

Event Timeline

paquette created this revision.Jun 2 2021, 5:52 PM
paquette requested review of this revision.Jun 2 2021, 5:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2021, 5:53 PM
aemerson added inline comments.Jun 3 2021, 10:21 AM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
6027

Can we simply use getFirstNonPHI() unconditionally?

paquette added inline comments.Jun 3 2021, 2:31 PM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
6027

No, because the point where we're inserting the COPY may be in a block without phis.

e.g. in test_loop_phi_gpr_to_fpr:

bb.1:
  successors: %bb.2(0x80000000)

  %6:gpr(s32) = G_IMPLICIT_DEF
  %7:gpr(s32) = G_SELECT %0(s1), %6, %6
  %1:gpr(s16) = G_TRUNC %7(s32)

bb.2:

In this case, OpDef is the G_TRUNC. So, we need to insert the copy after it (at the end of the block). So, inserting after the first non-phi instruction isn't correct here.

aemerson accepted this revision.Jun 4 2021, 9:21 AM

Ok, LGTM.

This revision is now accepted and ready to land.Jun 4 2021, 9:21 AM