This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Lower dbg.declare into indirect DBG_VALUE
ClosedPublic

Authored by cuviper on Sep 6 2018, 1:32 PM.

Details

Summary

D31439 changed the semantics of dbg.declare to take the address of a
variable as the first argument, making it indirect. It specifically
updated FastISel for this change here:

https://reviews.llvm.org/D31439#change-WVArzi177jPl

GlobalISel needs to follow suit, or else it will be missing a level of
indirection in the generated debuginfo. This problem was seen in a Rust
debuginfo test on aarch64, since GlobalISel is used at -O0 for aarch64.

https://github.com/rust-lang/rust/issues/49807
https://bugzilla.redhat.com/show_bug.cgi?id=1611597
https://bugzilla.redhat.com/show_bug.cgi?id=1625768

Diff Detail

Repository
rL LLVM

Event Timeline

cuviper created this revision.Sep 6 2018, 1:32 PM
cuviper edited the summary of this revision. (Show Details)Sep 6 2018, 1:33 PM
aprantl added a reviewer: rnk.Sep 6 2018, 1:46 PM
aprantl added a subscriber: debug-info.

Note that strictly speaking a dbg.declare is not 100% the same as an indirect DBG_VALUE. A dbg.declare declares a stack slot for a variable, and specifically has a lifetime valid for the entire lexical scope of that variable, whereas the lifetime of a DBG_VALUE only lasts until the next DBG_VALUE for the same variable, the end of the basic block (except where LiveDebugValues can prove it doesn't), or until the value the DBG_Value is pointing to is clobbered, whichever happens first.

Because of this the other instruction selectors enter dbg.declares into the MMI sidetable, which is a map of stack slots.

Because of LiveDebugValues this patch might still work though.

Does GlobalISel support the MMI side table? Otherwise this seems a fine approach.

rnk added a comment.Sep 6 2018, 2:07 PM

Because of LiveDebugValues this patch might still work though.

Does GlobalISel support the MMI side table? Otherwise this seems a fine approach.

It already uses the (now MachineFunction, no longer MMI) side table, if the argument is a static alloca. The original code with more context:

auto AI = dyn_cast<AllocaInst>(Address);
if (AI && AI->isStaticAlloca()) {
  // Static allocas are tracked at the MF level, no need for DBG_VALUE
  // instructions (in fact, they get ignored if they *do* exist).
  MF->setVariableDbgInfo(DI.getVariable(), DI.getExpression(),
                         getOrCreateFrameIndex(*AI), DI.getDebugLoc());
} else
  MIRBuilder.buildDirectDbgValue(getOrCreateVReg(*Address),
                                 DI.getVariable(), DI.getExpression());
return true;

So, this only affects variables that are declared in not-allocas at -O0, i.e. things passed indirectly with byval, or sret, or by address in the way that non-trivially copyable C++ types are.

This code could be improved to handle more cases where the value in question actually has a frame index. byval and inalloca are the obvious ways that I know of to get those. Looks like this is the code we use in SDAG ISel for it:

} else if (const auto *Arg = dyn_cast<Argument>(
               Address->stripInBoundsConstantOffsets())) {
  FI = FuncInfo.getArgumentFrameIndex(Arg);
}

I think this change could use a more direct test, consider starting with the IR generated for a C++ object, like:

struct NTCopy {
  NTCopy();
  NTCopy(const NTCopy &);
  int x;
};
int foo(NTCopy o) {
  return o.x;
}
test/CodeGen/AArch64/GlobalISel/debug-insts.ll
12

This test is strange, it declares the variable twice, once in %in.addr (correct) and again in %in (incorrect).

20

This change makes sense, though. The VLA is at some stack pointer offset. I assume p0 is the SP reg.

In D51749#1226417, @rnk wrote:

I think this change could use a more direct test, consider starting with the IR generated for a C++ object, like:

struct NTCopy {
  NTCopy();
  NTCopy(const NTCopy &);
  int x;
};
int foo(NTCopy o) {
  return o.x;
}

Thanks for this simple example -- it does indeed produce a bad DEBUG_VALUE of $x0 before, now fixed to an indirect [$x0+0]. I'll add this as a test.

test/CodeGen/AArch64/GlobalISel/debug-insts.ll
12

This test is strange, it declares the variable twice, once in %in.addr (correct) and again in %in (incorrect).

Yeah, I don't know why. Clang only produces the %in.addr version for void debug_declare(int in). Then you don't get any DBG_VALUE output for this function, but I think that's probably OK, and in.addr has CHECK lines above. I can remove the plain %in one.

20

I assume p0 is the SP reg.

Yes, the lines up to %6 compute the VLA size, and then:

$sp = COPY %6(p0)
%7:_(p0) = COPY %6(p0)
DBG_VALUE debug-use %7(p0), 0, !14, !DIExpression(), debug-location !15
cuviper updated this revision to Diff 164488.Sep 7 2018, 12:07 PM
  • Removed the duplicate (and incorrect) declaration in debug-insts.ll debug_declare
  • Added debug-cpp.ll which more directly targets this change.
cuviper marked 4 inline comments as done.Sep 7 2018, 12:08 PM
rnk accepted this revision.Sep 7 2018, 12:55 PM

lgtm

This revision is now accepted and ready to land.Sep 7 2018, 12:55 PM
This revision was automatically updated to reflect the committed changes.