[PATCH] D37768: [IR] Add llvm.dbg.addr, a control-dependent version of llvm.dbg.declare

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/docs/SourceLevelDebugging.rst:202
+    call void @llvm.dbg.addr(metadata [256 x i8]* %buffer, metadata !3,
+                             metadata !DIExpression(DW_OP_plus, 64))
+    !3 = !DILocalVariable(name: "i", ...) ; int i
----------------
please add a DILocation (`!dbg !4`), since it actually is an integral part of the intrinsic.


================
Comment at: llvm/docs/SourceLevelDebugging.rst:205
+
+A frontend should generate one call to ``llvm.dbg.addr`` at the point of
+declaration of a source variable. Optimization passes that fully promote the
----------------
"exactly one" ?


================
Comment at: llvm/docs/SourceLevelDebugging.rst:221
+This intrinsic is identical to `llvm.dbg.addr`, except that there can only be
+one call to `llvm.dbg.declare` for a given concrete `local variable
+<LangRef.html#dilocalvariable>`_. It is not control-dependent, meaning that if
----------------
I find this somewhat confusing for the reader since the above paragraph makes it sound like there can also only be one dbg.addr.


================
Comment at: llvm/docs/SourceLevelDebugging.rst:222
+one call to `llvm.dbg.declare` for a given concrete `local variable
+<LangRef.html#dilocalvariable>`_. It is not control-dependent, meaning that if
+a call to `llvm.dbg.declare` exists and has a valid location argument, that
----------------
control-flow-dependent?


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:130
+
+    // Methods for support type inquiry through isa, cast, and dyn_cast:
+    static bool classof(const IntrinsicInst *I) {
----------------
```
/// Methods for ...
/// @{
...
/// @}
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5144
 
-    // Byval arguments with frame indices were already handled after argument
-    // lowering and before isel.
-    if (const auto *Arg =
-            dyn_cast<Argument>(Address->stripInBoundsConstantOffsets()))
-      if (FuncInfo.getArgumentFrameIndex(Arg) != INT_MAX)
-        return nullptr;
+    // llvm.dbg.addr is control dependent and always generates indirect
+    // DBG_VALUE instructions. llvm.dbg.declare is handled as a frame index in
----------------
control flow ?


================
Comment at: llvm/lib/IR/DIBuilder.cpp:28
+cl::opt<bool>
+    UseDbgAddr("use-dbg-addr",
+                llvm::cl::desc("Use llvm.dbg.addr for all local variables"),
----------------
What would break (besides IR testcases) if we made this the default immediately?


================
Comment at: llvm/lib/IR/Verifier.cpp:4006
   case Intrinsic::dbg_value: // llvm.dbg.value
     visitDbgIntrinsic("value", cast<DbgInfoIntrinsic>(*CS.getInstruction()));
     break;
----------------
that string correct?


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4105
   // and the individual partitions.
-  if (DbgDeclareInst *DbgDecl = FindAllocaDbgDeclare(&AI)) {
-    auto *Var = DbgDecl->getVariable();
-    auto *Expr = DbgDecl->getExpression();
+  TinyPtrVector<DbgInfoIntrinsic *> DbgDeclares = FindDbgAddrUses(&AI);
+  if (!DbgDeclares.empty()) {
----------------
These are no longer just dbg.declares, are they?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1254
+
+  // Return nothing if the locations or DILocalVariables disagree.
+  DILocalVariable *Local =
----------------
Should this be a verifier error?


================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:106
   Value *AllocaPointerVal;
-  DbgDeclareInst *DbgDeclare;
+  TinyPtrVector<DbgInfoIntrinsic*> DbgDeclares;
 
----------------
These are no longer just dbg.declares, are they?


================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:248
   /// intrinsic if the alloca gets promoted.
-  SmallVector<DbgDeclareInst *, 8> AllocaDbgDeclares;
+  SmallVector<TinyPtrVector<DbgInfoIntrinsic *>, 8> AllocaDbgDeclares;
 
----------------
These are no longer just dbg.declares, are they?


https://reviews.llvm.org/D37768