[PATCH] D37018: [GISel]: Legalize G_PHIs

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

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



================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:432
   /// \return The newly created instruction.
   MachineInstrBuilder buildZExtOrTrunc(unsigned Res, unsigned Op);
+  // Build and insert \p Res<def> = G_ANYEXT \p Op, \p Res = G_TRUNC \p Op, or
----------------
Add a newline


================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:353
+          TargetOpcode::G_SEXT == ExtOpc) &&
+         "Expecting Extending Opc");
   assert(MRI->getType(Res).isScalar() || MRI->getType(Res).isVector());
----------------
Formatting looks weird. Is this clang-formatted?


================
Comment at: test/CodeGen/AArch64/GlobalISel/legalize-phi.mir:51
+liveins:         
+frameInfo:       
+  isFrameAddressTaken: false
----------------
You can get rid of the frameInfo, fixedStack, stack and constants sections


https://reviews.llvm.org/D37018