This is an archive of the discontinued LLVM Phabricator instance.

[globalopt] Change so that emitting fragments doesn't use the type size of DIVariables
ClosedPublic

Authored by akhuang on Aug 7 2020, 5:10 PM.

Details

Summary

When turning on -debug-info-kind=constructor we ran into a "fragment covers
entire variable" error during thinlto. The fragment is currently always
emitted if there is no type size, but sometimes the variable has a
forward declared struct type which doesn't have a size.

This changes the code to get the type size from the GlobalVariable instead.

Diff Detail

Event Timeline

akhuang created this revision.Aug 7 2020, 5:10 PM
akhuang requested review of this revision.Aug 7 2020, 5:10 PM
nickdesaulniers accepted this revision.Aug 10 2020, 2:44 PM
nickdesaulniers added a reviewer: DavidSpickett.

As long as llvm/test/DebugInfo/Generic/global-sra-struct-zero-length.ll passes with this, I'm happy. LGTM

llvm/test/DebugInfo/Generic/global-sra-struct-fwd-decl.ll
15–16

Is there a CHECK-NOT we can add too to check that we don't observe a fragment for d?

This revision is now accepted and ready to land.Aug 10 2020, 2:45 PM
akhuang added inline comments.Aug 10 2020, 4:17 PM
llvm/test/DebugInfo/Generic/global-sra-struct-fwd-decl.ll
15–16

There's already a check that the DIGlobalVariableExpression doesn't have a fragment. Do you mean a check to make sure there are no other DW_OP_LLVM_fragments emitted?

llvm/test/DebugInfo/Generic/global-sra-struct-fwd-decl.ll
15–16

With your test case, but not the above changes to LLVM, is there a CHECK-NOT case that could be added that would fail prior to your changes, but pass afterwards?

akhuang added inline comments.Aug 10 2020, 5:23 PM
llvm/test/DebugInfo/Generic/global-sra-struct-fwd-decl.ll
15–16

I checked that this test fails before my changes, from the existing CHECK: ![[GVE]] = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())

The logic LGTM, I wondered at the time I changed this if it was possible to not have a size but didn't know how to trigger it.

This revision was landed with ongoing or failed builds.Aug 11 2020, 2:51 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Aug 11 2020, 3:11 PM
llvm/test/DebugInfo/Generic/global-sra-struct-fwd-decl.ll
5

Is the source still the same as global-sra-struct-zero-length.ll?

MaskRay added inline comments.Aug 11 2020, 3:19 PM
llvm/test/DebugInfo/Generic/global-sra-struct-fwd-decl.ll
14

It is not "d". It is "d.0" (scalar replacement of aggregate: d.b)

akhuang added inline comments.Aug 11 2020, 3:45 PM
llvm/test/DebugInfo/Generic/global-sra-struct-fwd-decl.ll
5

I just copied global-sra-struct-zero-length.ll and changed struct d to be a declaration instead of a definition. So the source should be the same? Technically you wouldn't be able to get this IR from the source, though.

14

Oh, true. I can fix the comments.

hans added a subscriber: hans.Aug 19 2020, 4:32 AM

Should we merge this to 11.x?

I think that makes sense.

hans added a comment.Aug 19 2020, 9:40 AM

I think that makes sense.

Pushed as 9b0e9ed0ac5f9047538106c55c84082f12a1945c.