[PATCH] D133313: [Assignment Tracking][18/*] Account for assignment tracking in LICM

[Jeremy Morse via Phabricator via llvm-commits]

jmorse accepted this revision.
jmorse added a comment.
This revision is now accepted and ready to land.

LGTM; you might want to delete tbaa but it's not too important. What's the behaviour if there are multiple exit paths from the loop that store different values, like

  if (foo) {
    var = 1; break;
  } else if (bar) {
    var = 2; break;
  } ...

Presumably there'll be an exit block with a PHI, then a store -- and all the dbg.assigns for 'var' will point at that final store, but will have different value operands? (Just trying to work out what this would look like in my mind.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1890
+      if (!NewID) {
+        NewSI->mergeDIAssignID(Uses);
+        NewID = cast_or_null<DIAssignID>(
----------------
I feel it'd be helpful to have a comment here saying "NewSI will have it's DIAssignID set here if there are any labelled stores in Uses", or something, so that the reader can work out where the non-null DIAssignID comes from.


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/licm/merge.ll:17
+
+; CHECK: for.inc:
+;; Check that the stores have actually been removed from this block, otherwise
----------------
CHECK-LABEL? (and the next block check)


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/licm/multi-exit.ll:23-24
+;;
+;; Check that a store sunk into multiple exits retains its (their) dbg.assign,
+;; and that the new stores share the same DIAssignID.
+
----------------
This seems slightly un-natural as there aren't any loop exits?


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

https://reviews.llvm.org/D133313