[PATCH] D36880: [GSel]: Add a cleanup combiner to cleanup legalization artifacts

[Volkan Keles via Phabricator via llvm-commits]

volkan added a comment.

Thank you Aditya. I added a few more inline comments.



================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h:34
+                        SmallVectorImpl<MachineInstr *> &DeadInsts) {
+    if ( MI.getOpcode() != TargetOpcode::G_ANYEXT )
+      return false;
----------------
Could you clang-format this line?


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h:55
+
+    if ( MI.getOpcode() != TargetOpcode::G_ZEXT )
+      return false;
----------------
Same as above.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h:58
+    MachineInstr *DefMI = MRI.getVRegDef(MI.getOperand(1).getReg());
+    if ( DefMI->getOpcode() == TargetOpcode::G_TRUNC ) {
+      DEBUG(dbgs() << ".. Combine MI: " << MI;);
----------------
Same as above.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h:82
+
+    if (TargetOpcode::G_SEXT != MI.getOpcode())
+      return false;
----------------
Could you update this too? Just to make it consistent with the others.


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:131
+          NumNewInsns = 0;
+          MachineInstr *CurrInst = CombineList.pop_back_val();
+          if (!CurrInst)
----------------
aditya_nandakumar wrote:
> volkan wrote:
> > You do the combines after legalizing all of the recorded instructions, but a recorded instruction might be erased. So, I think we need to iterate over the MBB after the legalization.
> I've added a check where if the legalization response is Legalized, we should assume it's erased and remove from the combine list. Is this what you had in mind? Is there some case where this check wouldn't be sufficient?
The problem is  `LegalizerHelper::Legalized` doesn't mean all of the inserted instruction are legal. Targets can insert illegal instructions as they are going to be legalized anyway.


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:130
+                 I != E; ++I)
+              DEBUG(dbgs() << ".. .. New MI: "; WorkList[I]->print(dbgs()));
+#endif
----------------
`DEBUG(dbgs() << ".. .. New MI: " << *WorkList[I]);`


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:142
+          for (auto *DeadMI : DeadInstructions) {
+            DEBUG(dbgs() << ".. Erasing Dead Instruction ";
+                  DeadMI->print(dbgs()));
----------------
Same as above: `dbgs() << ... << *DeadMI`.


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:153
+                 I != E; ++I)
+              DEBUG(dbgs() << ".. .. Combine New MI: ";
+                    CombineList[I]->print(dbgs()));
----------------
Same as above.


https://reviews.llvm.org/D36880