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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
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
----------------
avl wrote:
> rnk wrote:
> > 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();`.
> > 
> That happens with this patch and llvm/test/DebugInfo/X86/sroa-after-inlining2.ll testcase if InstructionCombining.cpp:3163-3173 piece deleted.
> 
> opt sroa-after-inlining2.ll -sroa -instcombine -inline -instcombine -sroa -S -o -
> 
> before second instcombine pass there is following IR : 
> 
> 
> ```
> entry:
>   %result = alloca i64, align 8
>   %tmpcast = bitcast i64* %result to %struct.S1*     <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>   %0 = bitcast i64* %result to i8*, !dbg !24
>   call void @llvm.lifetime.start.p0i8(i64 8, i8* nonnull %0), !dbg !24
>   call void @llvm.dbg.declare(metadata %struct.S1* %tmpcast, metadata !12, metadata !DIExpression()), !dbg !25   <<<<<<<<<<
> ```
> 
> after second instcombine pass it transformed into : 
> 
> 
> ```
> entry:
>   %result = alloca i64, align 8
>   %0 = bitcast i64* %result to i8*, !dbg !24
>   call void @llvm.lifetime.start.p0i8(i64 8, i8* nonnull %0), !dbg !24
>   call void @llvm.dbg.declare(metadata i64* %result, metadata !12, metadata !DIExpression()), !dbg !25
> 
> 
> if.end:                                           ; preds = %entry
>   %tmpcast = bitcast i64* %result to %struct.S1*   <<<<<<<<<<<<<<<<<<<<<<<<<
>   call void @llvm.dbg.declare(metadata %struct.S1* %tmpcast, metadata !12, metadata !DIExpression()), !dbg !25   <<<<<<<<<<
> ```
> 
> instcombine moves bitcast instruction and clones dbg.declare.
(Looks like the `!isa<AllocaInst>(I)` allows you to detect the bitcast-of-alloc case.)

I'm not quite convinced by this example, but maybe there is something I'm missing, let me know!

The semantics of dbg.declare(alloca, var) are that the alloca is the definitive storage for var. There is no need to insert duplicate dbg.declares later in the code because a dbg.declare is valid throughout the function until the lifetime of alloca ends or var goes out of scope.
If we need more fine-grained tracking of locations we have dbg.value and dbg.addr at our disposal, which are valid until the next dbg.value/addr for the same (overlapping fragment of the) variable. LowerDbgDeclare is the pass that switches between these two modes by deleting the dbg.declare and inserting a dbg.value at every load from the alloca.

I suspect that the disconnect between my mental model and what this patch is about comes from that LowerDbgDeclare is good at describing the SSA values loaded from memory, but does a terrible job describing the values stored into alloca, as it is neither inserting a dbg.addr for store instuctions describing the alloca, nor are we inserting dbg.addr when the address is passed to function that takes a pointer aliasing with the variable.

To reiterate: I think it is a bug to have more than one dbg.declare describing the same variable fragment in the same alloca (see SourceLevelDebugging.rst, llvm.dbg.declare). If we need to duplicate something pointing to an alloca, it should be a dbg.value or dbg.addr. (Similarly, I also think it is a bug to have a dbg.declare and a dbg.value describing the same variable.)

For the quoted example, a single dbg.declare would be sufficient, assuming there are no other dbg.values for this variable. If there are dbg.values present outside of the quoted region, the dbg.declares should be dbg.value(%result, !DIExpression(DW_OP_deref)) instead.


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

https://reviews.llvm.org/D64595