[PATCH] D132223: [Assignment Tracking][4/*] Add llvm.dbg.assign intrinsic boilerplate

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Looks good with some hand waving in comments; as before, it's good practice to have tests covering the verifier failures.



================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:288-291
+  // Get the FragmentInfo for the variable. Note that dbg.assign intrinsics
+  // track two DIExpressions, but only the first expression has FragmentInfo.
+  // Using this function rather than going through DIExpression directly helps
+  // to abstract/hide that hack. FIXME: Move fragment info out of DIExpression.
----------------
This wants three slashes rather than two; I get the feeling the comment is a bit too verbose, in that it's writing a bit too much about one intrinsic in a generic class. IMO, better to focus on "this abstracts where that information is stored".


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:375
+    return I->getIntrinsicID() == Intrinsic::dbg_value ||
+           I->getIntrinsicID() == Intrinsic::dbg_assign;
+  }
----------------
Hmm -- so this is going to mean that feeding a dbg.assign into `isa<DbgValueInst>(...)` is true, doesn't that risk some misinterpretation out there?

If this is activating some code paths that are necessary for handling / accounting for debug intrinsics, we might want to handle that by putting a new superclass in rather than having dbg.values alias dbg.addrs.

(/me reads further) Ah, because dbg.addr subclasses dbg.value. I can see that there's precedent for this elsewhere (all these things are subclasses of CallInst), I suspect we'll need to think through carefully what the implications are.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:86
+      return false;
+    DAI->setAddress(NewValue);
+    return true;
----------------
Hmmmm. So this is called _inside_ an assertion, but it also has a side-effect / assignment... doesn't that mean we'll get different behaviour if LLVM is compiled with/without assertions?


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:94
   auto OldIt = find(Locations, OldValue);
-  assert(OldIt != Locations.end() && "OldValue must be a current location");
+  assert((OldIt != Locations.end() || DbgAssignAddrReplaced) &&
+         "OldValue must be a current location");
----------------
Missing () on the `DbgAssignAddrReplaced` call?


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:179-184
+void DbgAssignIntrinsic::replaceUsesOfWith(Value *From, Value *To) {
+  if (getValue() == From)
+    setValue(To);
+  if (getAddress() == From)
+    setAddress(To);
+}
----------------
Will this work with DIArgList's? (Probably, figured I'd ask though). Seeing how DbgAddrInst is acting as a DbgValueInst, AFAIUI the loop-strength-reduce and similar code will try and RAUW the value with a DIArgList of values occasionally. I think the semantics of this is obvious, the only risk is that we encode assumptions that the Value is only ever a single ValueAsMetadata.


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

https://reviews.llvm.org/D132223