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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar marked 11 inline comments as done.
aditya_nandakumar added a comment.

Thanks Volkan for your feedback. Replied/Updated to most comments.



================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:35
+                        SmallVectorImpl<MachineInstr *> &DeadInsts) {
+    if (TargetOpcode::G_ANYEXT != MI.getOpcode())
+      return false;
----------------
volkan wrote:
> `MI.getOpcode() != TargetOpcode::G_ANYEXT` would be better here.
This comes from my habit of using the Constant on the left idiom (old habit so I don't accidentally do if (a =42) foo() which will silently compile where I meant to say ==, but with constant on the left, it won't accidentally compile). While I don't think one way is better than the other, I have updated as requested :). 


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:38
+    MachineInstr *DefMI = MRI.getVRegDef(MI.getOperand(1).getReg());
+    if (TargetOpcode::G_TRUNC == DefMI->getOpcode()) {
+      DEBUG(dbgs() << ".. Combine MI: "; MI.print(dbgs()));
----------------
volkan wrote:
> - Missing check: `DefMI && ...`.
> - Same here: `DefMI->getOpcode() == TargetOpcode::G_TRUNC`.
> 
> Since you check the same condition in the other functions too, I think it would be better if you could add a function for this.
I didn't think the DefMI check here is necessary as it's impossible (illegal) to generate a G_[AZS]_EXT instruction where the input operand is not a register defined from an instruction. I can probably add it for completeness. Maybe an assert here is better?


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:112
+    unsigned SrcReg = MI.getOperand(NumDefs).getReg();
+    MachineInstr &MergeI = *MRI.def_instr_begin(SrcReg);
+    if (MergeI.getOpcode() != TargetOpcode::G_MERGE_VALUES)
----------------
volkan wrote:
> Could you replace this with `MRI.getVRegDef(SrcReg)` and check `if(MRI.getVRegDef(SrcReg))`?
Is the check really necessary vs an assert?


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


https://reviews.llvm.org/D36880