[PATCH] D132225: [Assignment Tracking][6/*] Add trackAssignments function
Jeremy Morse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 6 05:42:21 PDT 2022
jmorse added inline comments.
================
Comment at: llvm/include/llvm/IR/DIBuilder.h:899-901
+ /// \param LinkedInstr Must have a DIAssignID which will be used to link
+ /// the dbg.assign. The intrinsic is inserted after
+ /// this instruction.
----------------
Nit: starting with "Must have..." doesn't explain it's purpose -- IMHO, better to start with "Instruction with a DIAssignID to link with this dbg.assign...".
================
Comment at: llvm/include/llvm/IR/DIBuilder.h:908
+ /// NOTE: \p ValExpr carries the FragInfo for the
+ /// variable.
+ DbgAssignIntrinsic *insertDbgAssign(Instruction *LinkedInstr, Value *Val,
----------------
\param DL as well, if we're documenting all the arguments
================
Comment at: llvm/include/llvm/IR/DIBuilder.h:910
+ DbgAssignIntrinsic *insertDbgAssign(Instruction *LinkedInstr, Value *Val,
+ DILocalVariable &SrcVar,
+ DIExpression *ValExpr, Value *Addr,
----------------
Why a reference for DILocalVariable -- convention (as I understand it) elsewhere is to pass all metadata as pointers.
================
Comment at: llvm/include/llvm/IR/DebugInfo.h:233
+ }
+};
+/// Track assignments to \p Vars between \p Start and \p End.
----------------
\n
================
Comment at: llvm/include/llvm/IR/DebugInfo.h:235
+/// Track assignments to \p Vars between \p Start and \p End.
+/// FIXME: Backing storage shouldn't be limited to allocas only. Some local
+/// variables have their storage allocated by the calling function (addresses
----------------
I'd say TODO rather than FIXME,
================
Comment at: llvm/include/llvm/IR/DebugInfo.h:239
+void trackAssignments(
+ Function::iterator Start, Function::iterator End,
+ const DenseMap<const AllocaInst *, SmallSet<VarRecord, 2>> &Vars,
----------------
Possibly silly question, but does this ever get called with iterators other than begin() and end()? I don't get the feeling that a range of blocks is ever useful (is that how inlining inserts them, as a range?).
================
Comment at: llvm/include/llvm/IR/DebugInfo.h:240
+ Function::iterator Start, Function::iterator End,
+ const DenseMap<const AllocaInst *, SmallSet<VarRecord, 2>> &Vars,
+ const DataLayout &DL, bool DebugPrints = false);
----------------
IMO: it's better to typedef a name for this data structure, that avoids the over-use of auto in the future.
================
Comment at: llvm/include/llvm/IR/DebugInfo.h:243
+
+/// Describes properties of a store that has a static and size offset into a
+/// some base storage. Used by the getAssignmentInfo functions.
----------------
"a static and size offset" doesn't parse to me, was this "a static size and offset"?
================
Comment at: llvm/include/llvm/IR/Value.h:750-754
+ /// Strip off pointer casts and GEPs.
+ ///
+ /// Returns the original pointer value. If this is called on a non-pointer
+ /// value, it returns 'this'.
+ const Value *stripOffsets() const;
----------------
I want to say that there must be something in LLVM that already does this, but I can only see things that deal with inbound geps, or constants. Could I suggest a name like "stripAllOffsets", to give the reader a strong hint that this method drops / ignores a _lot_ of information -- that'll discourage people from using it in contexts where they need to care about some of the offsets. (Renaming can be in a follow-up patch or something, no need to re-juggle the patch stack because of a name nitpick).
================
Comment at: llvm/lib/IR/DIBuilder.cpp:958-965
+ std::array<Value *, 6> Args = {
+ StoredVal,
+ MetadataAsValue::get(Ctx, &SrcVar),
+ MetadataAsValue::get(Ctx, ValExpr),
+ MetadataAsValue::get(Ctx, Link),
+ Destination,
+ MetadataAsValue::get(Ctx, AddrExpr),
----------------
Aesthetics nit -- why not make the call to MetadataAsValue::get for StoredVal and Destination in this block, then it would all be beautifully aligned? (Feel free to ignore this style comment).
================
Comment at: llvm/lib/IR/DebugInfo.cpp:1693-1694
+
+/// Collect constant properies (base, size, offset) of \p StoreDest.
+/// Return None if any properties are not constants.
+static Optional<AssignmentInfo> getAssignmentInfoImpl(const DataLayout &DL,
----------------
Just to ask the obvious question: are you able to use `Value::stripAndAccumulateConstantOffsets` instead of this?
================
Comment at: llvm/lib/IR/DebugInfo.cpp:1735
+ // Assume 8 bit bytes.
+ auto *ConstOffset = dyn_cast<ConstantInt>(I->getOperand(2));
+ if (!ConstOffset)
----------------
Is this field actually an offset as the variable name suggests? My (shallow) reading of MemIntrinsic suggests that it's the length (aka size?) of the load/store being performed, which is not an offset.
================
Comment at: llvm/lib/IR/DebugInfo.cpp:1756
+
+static CallInst *emitDbgAssign(AssignmentInfo Info, Value &Val, Value &Dest,
+ Instruction &StoreLikeInst,
----------------
The call to emitDbgAssign added in this patch dereferences a pointer to Val and Dest, then the address is taken off them again further below. Better (IMHO) to just pass in the pointer, given that `insertDbgAssign` takes pointers?
================
Comment at: llvm/lib/IR/DebugInfo.cpp:1769
+ }
+ DIExpression *AddrExpr = DIExpression::get(StoreLikeInst.getContext(), None);
+ return DIB.insertDbgAssign(&StoreLikeInst, &Val, *VarRec.Var, Expr, &Dest,
----------------
Feels like you can just re-use the identical call to DIExpression::get from above.
================
Comment at: llvm/lib/IR/DebugInfo.cpp:1781-1783
+ // Early-exit if there are no interesting variables.
+ if (Vars.empty())
+ return;
----------------
Any need for "if Start == End return"?
================
Comment at: llvm/lib/IR/DebugInfo.cpp:1816
+ DestComponent = MI->getOperand(0);
+ } else if (auto *MI = dyn_cast<MemSetInst>(&I)) {
+ Info = getAssignmentInfo(DL, MI);
----------------
MemTransferInst instead? The distinction between `memcpy` and `memmove` isn't too important here.
================
Comment at: llvm/lib/IR/DebugInfo.cpp:1853-1858
+ DIAssignID *ID =
+ cast_or_null<DIAssignID>(I.getMetadata(LLVMContext::MD_DIAssignID));
+ if (!ID) {
+ ID = DIAssignID::getDistinct(Ctx);
+ I.setMetadata(LLVMContext::MD_DIAssignID, ID);
+ }
----------------
This can be hoisted out of the VarRecord loop, no?
================
Comment at: llvm/lib/IR/DebugInfo.cpp:1879
+ for (auto &I : BB) {
+ if (auto *DDI = dyn_cast<DbgDeclareInst>(&I)) {
+ // FIXME: trackAssignemnts doesn't let you specify any modifiers to the
----------------
To avoid extra indentation, dyn cast then "if (!DDI) continue;"?
================
Comment at: llvm/lib/IR/DebugInfo.cpp:1880
+ if (auto *DDI = dyn_cast<DbgDeclareInst>(&I)) {
+ // FIXME: trackAssignemnts doesn't let you specify any modifiers to the
+ // variable (e.g. fragment) or location (e.g. offset), so we have to
----------------
================
Comment at: llvm/lib/IR/DebugInfo.cpp:1910-1918
+ for (DbgDeclareInst *DDI : P.second) {
+ // FIXME: Are there any that wouldn't get converted now? Better to be
+ // safe and still check for now.
+ if (std::find_if(Markers.begin(), Markers.end(),
+ [DDI](DbgAssignIntrinsic *DAI) {
+ return DAI->getVariable() == DDI->getVariable();
+ }) != Markers.end())
----------------
Not completely clear what the find is doing, comment appreciated. Promote to an assertion perhaps, or EXPENSIVE_CHECKS?
================
Comment at: llvm/lib/IR/DebugInfo.cpp:1925-1926
+ runOnFunction(F);
+ // Q: Can we return a less conservative set than just CFGAnalyses? Can we
+ // return PreservedAnalyses::all()?
+ PreservedAnalyses PA;
----------------
IMHO: we should be able to specify that all analyses are preserved. If they're not, that's a debug-info-changes-codegen bug.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132225/new/
https://reviews.llvm.org/D132225
More information about the llvm-commits
mailing list