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

[Volkan Keles via Phabricator via llvm-commits]

volkan added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:1
+//===-- llvm/CodeGen/GlobalISel/LegalizerCombiner.h
+//-----------------------------===//
----------------
This doesn't match with the filename. Is it `LegalizerCombiner.h` or `LegalizerCombine.h`?


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:2
+//===-- llvm/CodeGen/GlobalISel/LegalizerCombiner.h
+//-----------------------------===//
+//
----------------
Could you remove the redundant dashes and format this line?


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:35
+                        SmallVectorImpl<MachineInstr *> &DeadInsts) {
+    if (TargetOpcode::G_ANYEXT != MI.getOpcode())
+      return false;
----------------
`MI.getOpcode() != TargetOpcode::G_ANYEXT` would be better here.


================
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()));
----------------
- 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.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:39
+    if (TargetOpcode::G_TRUNC == DefMI->getOpcode()) {
+      DEBUG(dbgs() << ".. Combine MI: "; MI.print(dbgs()));
+      unsigned DstReg = MI.getOperand(0).getReg();
----------------
What about `DEBUG(dbgs() << ".. Combine MI: " << *MI);`?


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:51
+    return false;
+  }
+  bool tryCombineZExt(MachineInstr &MI,
----------------
An empty line here would make the code easy to read.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:52
+  }
+  bool tryCombineZExt(MachineInstr &MI,
+                      SmallVectorImpl<MachineInstr *> &DeadInsts) {
----------------
Same as above.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:76
+    return false;
+  }
+  bool tryCombineSExt(MachineInstr &MI,
----------------
An empty line here would make the code easy to read.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:77
+  }
+  bool tryCombineSExt(MachineInstr &MI,
+                      SmallVectorImpl<MachineInstr *> &DeadInsts) {
----------------
Same as above.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:107
+                        SmallVectorImpl<MachineInstr *> &DeadInsts) {
+    if (MI.getOpcode() != TargetOpcode::G_UNMERGE_VALUES)
+      return false;
----------------
Same as above.


================
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)
----------------
Could you replace this with `MRI.getVRegDef(SrcReg)` and check `if(MRI.getVRegDef(SrcReg))`?


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombine.h:178
+    return true;
+  }
+  /// Try to combine away MI.
----------------
An empty line here would make the code easy to read.


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


https://reviews.llvm.org/D36880