[PATCH] D132220: [Assignment Tracking][1/*] Add initial docs for Assignment Tracking

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Not a major nit, but something occasionally encountered: IMO the word "debug" needs to occur somewhere in the opening paragraph, or even title. The compiler as a whole covers a pretty wide range of topics, and someone stumbling across the page at random might not immediately know that it's about debug-info.

Sounds good; not hitting "accept" just yet until we've waved these patches in the usual directions.



================
Comment at: llvm/docs/AssignmentTracking.md:5-7
+assignments where a local variable (or a field of one) is the LHS. Indirect
+assignments that get optimized out may not be visible, but the locations should
+otherwise be reasonable.
----------------
IMO this could be interpreted quite widely -- I think the point is to indicate that the technique isn't perfectly 100% sound, in which case perhaps we could say "in rare and complicated circumstances indirect assignments might be optimized away without being tracked, but otherwise we make our best effort to track all variable locations". The word "reasonable" does, alas, mean different things to different people.


================
Comment at: llvm/docs/AssignmentTracking.md:12
+use non-memory locations (register, constant) or memory locations until after
+middle end optimisations have run. This is in opposition to what we have now,
+which is to make the decision for most variables early on, which can result in
----------------
"what we have now" will immediately go stale, how about "to using dbg.declare and dbg.value"


================
Comment at: llvm/docs/AssignmentTracking.md:49-57
+This is the actual signature of an `llvm.dbg.assign` in IR:
+```
+void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata)
+```
+
+That isn't particularly illuminating, so here's the signature in practice. Each
+parameter is wrapped in `MetadataAsValue`, and `Value *` type parameters are
----------------
Probably best to ditch this IMO; or put the 2nd signature first and say "the formal signature in LLVM-IR is <metadata*5>...". That avoids the reader thinking they're reading redundant things.


================
Comment at: llvm/docs/AssignmentTracking.md:75-77
+`DIAssignID` metadata is the mechanism that is currently used to encode the
+store<->marker link. It has no operands and all instances are `distinct`;
+equality is checked for by comparing addresses.
----------------
I'd suggest fitting the word "node" in here somewhere just to be explicit that this is part of the existing DI metadata hierarchy, rather than some other meta-metadata.


================
Comment at: llvm/docs/AssignmentTracking.md:111
+
+The first `llvm.dbg.asssign` refers to the `alloca` through `!DIAssignID !13`,
+and the second refers to the `store` through `!DIAssignID !16`.
----------------
asssign -> assign


================
Comment at: llvm/docs/AssignmentTracking.md:127
+
+Considerations for pass writers and maintainers
+**cloning** an instruction: nothing new to do. Cloning automatically clones a
----------------
Dunno how the elements below render, but presumably they want capitalization ("Cloning", "Moving" etc).

Also IMO, it's worth moving moving / deleting debug instructions to their own list, so that there's a collection of "things to do to Real instruction" and "things to do to debug instructions", which decomposes a little nicer IMHO, YMMV.


================
Comment at: llvm/docs/AssignmentTracking.md:190
+the choice at each instruction, iteratively joining the results for each block.
+
----------------
I think we've discussed existing limitations elsewhere -- does it make sense to put a few in here as a very high level "TODO" list, as it were? No point if it's a substantial amount of work, but if there's already a list, that could help future readers.


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

https://reviews.llvm.org/D132220