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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar marked 7 inline comments as done.
aditya_nandakumar added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:131
+          NumNewInsns = 0;
+          MachineInstr *CurrInst = CombineList.pop_back_val();
+          if (!CurrInst)
----------------
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.


https://reviews.llvm.org/D36880