[PATCH] D61787: [GlobalISel Legalizer] Improve artifact combiner

[Simon Atanasyan via Phabricator via llvm-commits]

atanasyan added a comment.

LGTM with a few nits. But I'm not an expert in GlobalISel so get reply from other reviewers.



================
Comment at: include/llvm/CodeGen/GlobalISel/GISelWorkList.h:86
+  bool contains(const MachineInstr *I) {
+    auto It = WorklistMap.find(I);
+    if (It == WorklistMap.end())
----------------
That can be written as `return WorklistMap.count(I)`.


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:307
+            MachineInstr *UseInst = MRI.getVRegDef(UseOp.getReg());
+            // Use is an unresolved artifact or is in InstList.
+            // InstList now contains instructions that used to be artifacts or
----------------
s/Use/UseInst/ in the comment?


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:336
+    // like instructions. Do this after we have processed everything else.
+    if (InstList.empty() && ArtifactList.empty() &&
+        TryLaterArtifactList.empty()) {
----------------
It looks like if move this block out of the `while` loop we can drop the `if` statement and write it as:
```
SmallVector<MachineInstr *, 8> Flip;
while (!AuxiliaryArtifactList.empty())
  Flip.push_back(AuxiliaryArtifactList.pop_back_val());
while (!Flip.empty())
  InstList.insert(Flip.pop_back_val());
```


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61787/new/

https://reviews.llvm.org/D61787