[PATCH] D71448: [Legalizer] Making artifact combining order-independent

[Jessica Paquette via Phabricator via llvm-commits]

paquette accepted this revision.
paquette added a comment.

LGTM aside from some places where a couple comments can be added.



================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h:481
+
+    SmallVector<Register, 4> UpdatedDefs;
+    bool Changed = false;
----------------
Can we have a comment explaining what the purpose of `UpdatedDefs` is?

E.g. "Store the refs that were changed by a combine, so that we can..." 


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h:503-504
+      Changed = tryCombineTrunc(MI, DeadInsts, UpdatedDefs);
+      if (!Changed)
+        UpdatedDefs.push_back(MI.getOperand(0).getReg());
+      break;
----------------
It's not immediately obvious to me why this is only happening for G_TRUNC. A comment would be helpful here. :)


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h:507
     }
+    while (!UpdatedDefs.empty()) {
+      Register NewDef = UpdatedDefs.pop_back_val();
----------------
A comment explaining what you're doing here at a high level would be nice too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71448