[PATCH] D39267: [GISel]: Change Legalization from top down to bottom up + DCE

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

Hi Aditya,

This looks like a nice re-work. I have some minor comments. Since there's quite a bit of test churn, it's not clear to me what the test case (if any) applies to the new tryFoldImplicitDef() code.

Cheers,
Amara



================
Comment at: include/llvm/CodeGen/GlobalISel/GISelWorkList.h:22
+// move all elements over one place - instead just nulls out the element of the vector.
+// FIXME: Does it make sense to factor out common code with the instcombinerWorkList?
+template<unsigned N>
----------------
I thought the same with my original patch. I think this is fine to live here for now though especially if logging needs to be different.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h:112
+
+  bool tryFoldImplicitDef(MachineInstr &MI,
+                          SmallVectorImpl<MachineInstr *> &DeadInsts) {
----------------
Can you add some comments explaining why this is needed/what it's trying to achieve.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h:264
+      auto SrcTy = MRI.getType(SrcReg);
+      if (SrcTy.isValid() && SrcTy == DstTy)
+        DefMI = MRI.getVRegDef(SrcReg);
----------------
A bit more concise if you test the inverse:
```
if (!SrcTy.isValid() || SrcTy != DstTy)
  break;
DefMI = MRI.getVRegDef(SrcReg)
```


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:88
+  GISelWorkList<128> ArtifactList;
+  ReversePostOrderTraversal<MachineFunction *> RPOT(&MF);
+  for (auto *MBB : RPOT) {
----------------
I think it would be helpful to add a clarifying comment here about why we're doing bottom up traversal, and also that we're now using RPO because of the way we use the worklist below. I.e. populating the worklist top down and popping from back.


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:129
       // and are assumed to be legal.
-      if (!isPreISelGenericOpcode(MI->getOpcode()))
+      if (!isPreISelGenericOpcode(MI.getOpcode()))
         continue;
----------------
Do we still need this? If we only ever legalise these and the artefacts are always generic opcodes then can we move this check into the worklist population loop and leave an assert here?


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:164
+      // need special handling. Add it to InstList, so when it's processed
+      // there, it has to be legal or specially handled.
+      else
----------------
Is there a specific test case?


https://reviews.llvm.org/D39267