This is an archive of the discontinued LLVM Phabricator instance.

Move the IsIndirect flag from DIVariable into DIExpression.
ClosedPublic

Authored by aprantl on Jan 14 2015, 5:19 PM.

Details

Summary

This has been long overdue, and is generally totally mechanical. It does break IR-Metadata compatibility, but we do that a lot at the moment :-)

In this form I'm taking no prisoners and simple reflow DIDescriptor::FlagLValueReference and FlagRValueReference to close the gap from removing the flag, but I could understand if someone rather wants to leave a hole in the enum.

Note: LLVM+CFE is rolled into one patch.

Commit message:

Move the IsIndirect flag from DIVariable into DIExpression.
This is a much more natural place for this information to be, but there
is also a more technical reason:

The IsIndirect flag is used to mark a variable that is turned
into a reference by virtue of the calling convention; this happends
for example to aggregate return values.
The inliner, for example, may actually need to undo this indirection to
correctly represent the value in its new context. This is impossible to
implement because the DIVariable can't be safely modified. We can however
safely construct a new DIExpression on the fly.

Diff Detail

Event Timeline

aprantl updated this revision to Diff 18199.Jan 14 2015, 5:19 PM
aprantl retitled this revision from to Move the IsIndirect flag from DIVariable into DIExpression..
aprantl updated this object.
aprantl edited the test plan for this revision. (Show Details)
aprantl added reviewers: dblaikie, echristo.
aprantl set the repository for this revision to rL LLVM.
aprantl added a subscriber: Unknown Object (MLST).
dblaikie edited edge metadata.Jan 16 2015, 10:06 AM

(might be easier to review/commit by adding the expression handling then just removing the flag as dead code afterwards, but I'm not sure - it's not bad the way you've got it either)

lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
237

Do we need MachineLocation::isIndirect anymore?

Hmm, I guess I do vaguely recall this being here before I added the other indirect flag... but I forget what it might be for, etc.

aprantl added a comment.EditedJan 16 2015, 10:51 AM

(might be easier to review/commit by adding the expression handling then just removing the flag as dead code afterwards, but I'm not sure - it's not bad the way you've got it either)

Oh that would be great, but it's complicated:
After having read http://reviews.llvm.org/D7016, look at this gem from SelectionDAGBuilder:

uint64_t Offset = DI->getOffset();
  // A dbg.value for an alloca is always indirect.
  bool IsIndirect = isa<AllocaInst>(V) || Offset != 0;
  SDDbgValue *SDV;
  if (Val.getNode()) {
    if (!EmitFuncArgumentDbgValue(V, Variable, Expr, Offset, IsIndirect,
                                  Val)) {
      SDV = DAG.getDbgValue(Variable, Expr, Val.getNode(), Val.getResNo(),
                            IsIndirect, Offset, dl, DbgSDNodeOrder);
      DAG.AddDbgValue(SDV, Val.getNode(), false);
    }
  } else

It is rematerializing the OP_deref that this patch removes in the form a of an indirect MachineLocation.

Horrible, right?

We could get rid of the indirect field in MachineLocation, if we were to redefine alloca's to be treated as addresses rather than values in dbg.value/dbg.declare.
Let me give an example. Currently, the following code:

%x = alloca i64
call void @llvm.dbg.declare(metadata %i64* %x, [var=x], [expr=[]])
store i64 42, %x

is lowered into

%x = i64 42
call void @llvm.dbg.value(metadata %i64* %x, [var=x], [expr=[]])

Observe that the alloca, although it is treated like an address in IR, is treated like it was the value as far as the debug info is concerned.
If we want to fix this we'd have to change the frontend to emit

%x = alloca i64
call void @llvm.dbg.declare(metadata %i64* %x, [var=x], [expr=[DW_OP_deref]])
store i64 42, %x

which could then be lowered into the same code as above. After separating out DIExpression from DIVariable this is really easy to do; before it was pretty much impossible.

I think I'd love to do that instead. Should we go break stuff? ;-)

[I also feel that need to apologize for adding the indirect field to MachineLocation in the first place; but I was much more reluctant to changing the IR semantics back then than I am now.]

aprantl accepted this revision.Feb 4 2015, 2:12 PM
aprantl added a reviewer: aprantl.

that was committed in r226476.

This revision is now accepted and ready to land.Feb 4 2015, 2:12 PM
aprantl closed this revision.Aug 18 2015, 10:42 AM

that was committed in r226476.