[PATCH] D144732: [Assignment Tracking] Elide a map copy in some cases

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

Broadly looks good, just a couple questions and a nit comment.



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1777
 
-    auto PredLiveOut = LiveOut.find(Pred);
-    // Pred must have been processed already. See comment at start of this loop.
-    assert(PredLiveOut != LiveOut.end());
+  // No preds visted yet.
+  if (VisitedPreds.empty()) {
----------------
visted->visited


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1788
+    if (CurrentLiveInEntry == LiveIn.end()) {
+      LiveIn[&BB] = PredLiveOut;
+      return /*Changed*/ true;
----------------
Tiny question - does using `LiveIn[&BB] = ...` here mean that an empty map will first be constructed in the `&BB` bucket before we assign to it? If that is the case it might be better to just use `insert` or `try_emplace` instead; don't think this is high impact either way.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1823-1824
   // (expensive check).
   if (CurrentLiveInEntry == LiveIn.end() ||
       BBLiveIn != CurrentLiveInEntry->second) {
     LiveIn[&BB] = std::move(BBLiveIn);
----------------
I see that above for the single-visited-pred case you've split this `if` into two separate checks, so that you can use the found iterator `CurrentLiveInEntry` if it's valid. Since we're doing essentially the same thing here, would it make sense to use the same logic in both places, or is there a reason for the difference?


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

https://reviews.llvm.org/D144732