[PATCH] D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In D64595#1621077 <https://reviews.llvm.org/D64595#1621077>, @avl wrote:

> Thus, for the current moment I think to implement your original suggestion with avoiding dbg.declare lowering in instcombiner for aggregates.


This more or less gets the behavior of `-instcombine-lower-dbg-declare=0` for structs, which is really all I cared about when I added it. The common case this flag improved is cases like the one in your test, simple C++ classes (like std::vector) stored on the stack and passed to some non-inline method call (__push_back_slow). So, I'm in favor of your change as written. In fact, I'd go further and remove the cl::opt if this lands, but let me do that since then I have to go update chromium's build to not pass it. :)



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3163-3164
     if (DII->getParent() == SrcBlock) {
-      // dbg.value is in the same basic block as the sunk inst, see if we can
-      // salvage it. Clone a new copy of the instruction: on success we need
-      // both salvaged and unsalvaged copies.
-      SmallVector<DbgVariableIntrinsic *, 1> TmpUser{
-          cast<DbgVariableIntrinsic>(DII->clone())};
-
-      if (!salvageDebugInfoForDbgValues(*I, TmpUser)) {
-        // We are unable to salvage: sink the cloned dbg.value, and mark the
-        // original as undef, terminating any earlier variable location.
-        LLVM_DEBUG(dbgs() << "SINK: " << *DII << '\n');
-        TmpUser[0]->insertBefore(&*InsertPos);
-        Value *Undef = UndefValue::get(I->getType());
-        DII->setOperand(0, MetadataAsValue::get(DII->getContext(),
-                                                ValueAsMetadata::get(Undef)));
+      if (DII->getIntrinsicID() == Intrinsic::dbg_declare &&
+          !isa<AllocaInst>(I)) {
+        // There should be only one dbg.declare instruction. Thus we could not
----------------
aprantl wrote:
> jmorse wrote:
> > Interesting -- are there circumstances where this happens (dbg.declare of something that isn't an alloca)? I suspect it would be IR that didn't make sense, we might be better off disallowing it in the IR Verifier instead.
> I must admit I'm confused how this patch relates to the example in the review description. What is the instruction coming in here in the example?
clang emits dbg.declare of things that are not allocas from time to time. Usually it is for C++ classes that, when passed by value at the source level, are instead passed indirectly by pointer as required by the ABI. Or, for POD structs passed using byval.

Also, after inlining, these arguments may resolve to pointers to non-alloca locations, as in `this->field = getSRetThing();`.



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1381
   return AI->isArrayAllocation() ||
     AI->getType()->getElementType()->isArrayTy();
 }
----------------
Perhaps this should be updated to use getAllocatedType in favor of getType()->getElementType() for consistency and to address future typeless pointers in IR.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1386
+static bool isStructure(AllocaInst *AI) {
+  return AI->getAllocatedType() && AI->getAllocatedType()->isStructTy();
+}
----------------
Surely getAllocatedType can never return null?


================
Comment at: llvm/test/Transforms/InstCombine/lower-dbg-declare.ll:2
 ; RUN: opt -instcombine < %s -S | FileCheck %s
+; XFAIL: *
 
----------------
I think you should update this test case to check the new correct dbg.declare that should remain.


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

https://reviews.llvm.org/D64595