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

[Volkan Keles via Phabricator via llvm-commits]

volkan added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:131
+          NumNewInsns = 0;
+          MachineInstr *CurrInst = CombineList.pop_back_val();
+          if (!CurrInst)
----------------
aditya_nandakumar wrote:
> volkan wrote:
> > 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.
> For each iteration of legalizeInstr, we get a return value Legalized (if legalized) and we assume that it's dead and remove from the combine list. When the additional instructions are legalized, depending on the legalizeAction, it will be Legalized (and removed from the CombineList) - or else combined.
I see now, thanks for explaining. That check should be enough then.


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:170
+      SmallVector<MachineInstr *, 4> DeadInsts;
+      Changed |= C.tryCombineMerges(*MI, DeadInsts);
+      for (auto *DeadMI : DeadInsts)
----------------
Do we still need this code block? I think the `tryCombineInstruction` call above already handles this case.


https://reviews.llvm.org/D36880