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

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 5 inline comments as done.
rnk added inline comments.


================
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
----------------
aprantl wrote:
> I find this somewhat confusing for the reader since the above paragraph makes it sound like there can also only be one dbg.addr.
Hm, the above paragraph was meant to clarify that the frontend should generate one call, but optimizations may introduce more dbg.addrs. Hopefully now it's clear?


================
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
----------------
aprantl wrote:
> control-flow-dependent?
"control dependence" is a pretty common compiler concept, I think it's clearer to keep this short.


================
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"),
----------------
aprantl wrote:
> What would break (besides IR testcases) if we made this the default immediately?
Our -O0 DWARF would probably become much larger and much worse. llvm.dbg.addr always generates DBG_VALUE instructions that may be lost, and even if they survive CodeGen, they usually lead to the creation of a single entry location list, which is larger than an inline exprloc.

We could work around this by using the MF side table if there is exactly one llvm.dbg.addr and optnone is set, but I want to see what the quality degradation is first to see if it is viable to improve DwarfDebug.cpp to standardize on DBG_VALUE and eliminate the side table.


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


================
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()) {
----------------
aprantl wrote:
> These are no longer just dbg.declares, are they?
The naming is inconsistent here and elsewhere. For this transition period when they can be either addrs or declares, I'd rather leave the naming alone until we flip the default and standardize on dbg.addr.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1254
+
+  // Return nothing if the locations or DILocalVariables disagree.
+  DILocalVariable *Local =
----------------
aprantl wrote:
> Should this be a verifier error?
Actually, I think this is bug. A single alloca may hold multiple variables at different offsets. I tried to construct a unit test for this, but it was difficult and fragile so I deleted it. There's also no correctness issue with multiple variables living at the same location in memory, either.

I tried to put together a real-world test case with inalloca on windows x86:
```
struct NonTrivial {
  NonTrivial() : x(1) {}
  NonTrivial(const NonTrivial &) = default;
  ~NonTrivial() {}
  int x;
};
static int inalloca(NonTrivial a, int b, int c) {
  return a.x + b + c;
}
extern "C" int f() {
  return inalloca(NonTrivial(), 2, 3);
}
```
Clang will generate a single alloca to hold a, b, and c, and I would like it if we could inline and SROA away everything while tracking their values (1, 2, and 3). However, there are compounding issues here that move this out of scope for this patch.


https://reviews.llvm.org/D37768