This is an archive of the discontinued LLVM Phabricator instance.

[GISel]: Legalize G_PHIs
ClosedPublic

Authored by aditya_nandakumar on Aug 22 2017, 11:50 AM.

Details

Summary

Implemented WidenScalar for G_PHI.

Diff Detail

Event Timeline

qcolombet edited edge metadata.Aug 22 2017, 4:31 PM

Hi Aditya,

Looks mostly good.
Couple of comments inlined + we need more tests and reduce them (e.g., remove the frameInfo stuff and so on).

Cheers,
-Quentin

lib/CodeGen/GlobalISel/LegalizerHelper.cpp
668

NonTermIt may not be an instruction, thus we cannot dereference it.

  • Use setInsertPt instead.
  • Add a test case with an empty predecessor block
676

No bracket for one line block

690

Can't we merge the two loops and get rid of the temporary NewRegs.

aditya_nandakumar marked 3 inline comments as done.

Updated based on Quentin's feedback.

Hi Aditya,

The code looks good to me. Minor nitpicks inlined.

Regarding the test cases, could you:

  • Add a test case with a cycle on a phi (e.g., a = phi a)
  • Add a test case with an empty predecessor
  • Add a comment before every test case explaining what we are checking
  • Add a test case for s8
  • Add a test case with def feeding into two different phis:
    • In the same destination block
      • with the same type
      • with different types
    • In different basic block (one on the true branch, one on the false branch)
      • with the same type
      • with different types

I know that most of these test cases are going to exercise the same code path, but I still like to add more and stranger tests to make sure still things pass if we change the implementation underneath. For instance if we start doing CSE when creating the EXTs.

Cheers,
-Quentin

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
433

Add a newline

lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
353

Formatting looks weird. Is this clang-formatted?

test/CodeGen/AArch64/GlobalISel/legalize-phi.mir
52

You can get rid of the frameInfo, fixedStack, stack and constants sections

aditya_nandakumar marked 2 inline comments as done.Aug 24 2017, 12:50 PM

I see that the test cases I thought I updated (and added) are not there (and can't find them in my tree). I'll recreate them and add them to the review again.

lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
353

Yup. This is clang-formatted..

Updated with removed nitpicks, more tests.

I miss these test cases:

  • Add a test case with def feeding into two different phis:
    • In the same destination block
    • In different basic block (one on the true branch, one on the false branch)

In other words:
bb1:
def =
...
bb2:
... = phi def
... = phi def

and
def =
...
condbr bb2, b33
bb2
... = phi def
bb3
... = phi def

Added 2 more tests (This caught a bug where we broke the invariant of PHI being the first in the BB). Fixed.

qcolombet accepted this revision.Aug 24 2017, 4:44 PM

LGTM.

Few more nitpicks, no need for more reviews.

lib/CodeGen/GlobalISel/LegalizerHelper.cpp
687

Haha, nice catch!

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

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.)

336

Could you check that we insert the G_TRUNC at the right place?

403

Ditto.

This revision is now accepted and ready to land.Aug 24 2017, 4:44 PM
aditya_nandakumar marked 2 inline comments as done.Aug 24 2017, 8:49 PM
aditya_nandakumar added inline comments.
lib/CodeGen/GlobalISel/LegalizerHelper.cpp
687

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 :)

test/CodeGen/AArch64/GlobalISel/legalize-phi.mir
122

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.

committed in r311763.