[PATCH] D136320: [Assignment Tracking Analysis][1/*] Add analysis pass core

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:915
+
+AssignmentTrackingLowering::LocMap
+AssignmentTrackingLowering::joinLocMap(const LocMap &A, const LocMap &B) {
----------------
Note to self, returning a DenseMap by value is probably fine if it gets NRVO'd.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:932
+  // remaining elements to into SymmetricDifference.
+  for (auto Pair : A) {
+    VariableID Var = Pair.first;
----------------
`auto &` or you might get storage.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:975
+    return Assignment::makeNoneOrPhi();
+  if (A.Status == Assignment::NoneOrPhi)
+    return Assignment::makeNoneOrPhi();
----------------
Coming immediately after the leading comment, shouldn't this also check if `A.Status == NoneOrPhi`?


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1087
+    else
+      BBLiveIn = joinBlockInfo(BBLiveIn, PredLiveOut->second);
+    FirstJoin = false;
----------------
/me squints -- I want to say that BBLiveIn being the argument and the return value will lead to some kind of aliasing weirdness or performance slowdown (in the form of un-necessary DenseMap copies). I can't actually put my finger on why that would be forced though. Do you have any feeling for it?


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1191
+
+  // The general structure here is inspired by VarLocBasedImp.cpp
+  // (LiveDebugValues).
----------------



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

https://reviews.llvm.org/D136320