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

[akhuang]

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.

[nickdesaulniers]

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?

[akhuang]

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?

[nickdesaulniers]

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]

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())

[DavidSpickett]

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.

[MaskRay]

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]

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]

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]

Should we merge this to 11.x?

[akhuang]

I think that makes sense.

[hans]

In D85572#2226368, @akhuang wrote:

I think that makes sense.

Pushed as 9b0e9ed0ac5f9047538106c55c84082f12a1945c.

llvm/lib/Transforms/IPO/GlobalOpt.cpp

if (isa<StructType>(Init->getType())) {
     return false;
 
   return GlobalUsersSafeToSRA(GV);
 }
 
 /// Copy over the debug info for a variable to its SRA replacements.
 static void transferSRADebugInfo(GlobalVariable *GV, GlobalVariable *NGV,
                                  uint64_t FragmentOffsetInBits,
-                                 uint64_t FragmentSizeInBits) {
+                                 uint64_t FragmentSizeInBits,
+                                 uint64_t VarSize) {
   SmallVector<DIGlobalVariableExpression *, 1> GVs;
   GV->getDebugInfo(GVs);
   for (auto *GVE : GVs) {
     DIVariable *Var = GVE->getVariable();
-    Optional<uint64_t> VarSize = Var->getSizeInBits();
-
     DIExpression *Expr = GVE->getExpression();
     // If the FragmentSize is smaller than the variable,
     // emit a fragment expression.
-    // If the variable size is unknown a fragment must be
-    // emitted to be safe.
-    if (!VarSize || FragmentSizeInBits < *VarSize) {
+    if (FragmentSizeInBits < VarSize) {
       if (auto E = DIExpression::createFragmentExpression(
               Expr, FragmentOffsetInBits, FragmentSizeInBits))
         Expr = *E;
       else
         return;
     }
     auto *NGVE = DIGlobalVariableExpression::get(GVE->getContext(), Var, Expr);
     NGV->addDebugInfo(NGVE);
   }
 }
 
 /// Perform scalar replacement of aggregates on the specified global variable.
 /// This opens the door for other optimizations by exposing the behavior of the
 /// program in a more fine-grained way.  We have determined that this
 /// transformation is safe already.  We return the first global variable we
 /// insert so that the caller can reprocess it.
 static GlobalVariable *SRAGlobal(GlobalVariable *GV, const DataLayout &DL) {
   // Make sure this global only has simple uses that we can SRA.
   if (!CanDoGlobalSRA(GV))
     return nullptr;
 
   assert(GV->hasLocalLinkage());
   Constant *Init = GV->getInitializer();
   Type *Ty = Init->getType();
+  uint64_t VarSize = DL.getTypeSizeInBits(Ty);
 
   std::map<unsigned, GlobalVariable *> NewGlobals;
 
   // Get the alignment of the global, either explicit or target-specific.
   Align StartAlignment =
       DL.getValueOrABITypeAlignment(GV->getAlign(), GV->getType());
 
   // Loop over all users and create replacement variables for used aggregate
if (StructType *STy = dyn_cast<StructType>(Ty)) {
       uint64_t FieldOffset = Layout.getElementOffset(ElementIdx);
       Align NewAlign = commonAlignment(StartAlignment, FieldOffset);
       if (NewAlign > DL.getABITypeAlign(STy->getElementType(ElementIdx)))
         NGV->setAlignment(NewAlign);
 
       // Copy over the debug info for the variable.
       uint64_t Size = DL.getTypeAllocSizeInBits(NGV->getValueType());
       uint64_t FragmentOffsetInBits = Layout.getElementOffsetInBits(ElementIdx);
-      transferSRADebugInfo(GV, NGV, FragmentOffsetInBits, Size);
+      transferSRADebugInfo(GV, NGV, FragmentOffsetInBits, Size, VarSize);
     } else {
       uint64_t EltSize = DL.getTypeAllocSize(ElTy);
       Align EltAlign = DL.getABITypeAlign(ElTy);
       uint64_t FragmentSizeInBits = DL.getTypeAllocSizeInBits(ElTy);
 
       // Calculate the known alignment of the field.  If the original aggregate
       // had 256 byte alignment for example, something might depend on that:
       // propagate info to each field.
       Align NewAlign = commonAlignment(StartAlignment, EltSize * ElementIdx);
       if (NewAlign > EltAlign)
         NGV->setAlignment(NewAlign);
       transferSRADebugInfo(GV, NGV, FragmentSizeInBits * ElementIdx,
-                           FragmentSizeInBits);
+                           FragmentSizeInBits, VarSize);
     }
   }
 
   if (NewGlobals.empty())
     return nullptr;
 
   Module::GlobalListType &Globals = GV->getParent()->getGlobalList();
   for (auto NewGlobalVar : NewGlobals)

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

+; RUN: opt -S -globalopt < %s | FileCheck %s
+; Generated at -O2 -g from:
+; typedef struct {} a;
+; static struct {
+;     long b;
+;     a c;
+; } d;
+; e() {
+;     long f = d.b + 1;
+;     d.b = f;
+; }
+; (IR is modified so that d's struct type is forward declared.)
+
+; Check that the global variable "d" is not
+; emitted as a fragment if its struct type is
+; forward declared but d.c has zero length, so
+; a fragment shouldn't be emitted.
+
+source_filename = "t.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%struct.anon = type { i64, %struct.a }
+%struct.a = type {}
+
+; CHECK: @d.0 = internal unnamed_addr global i64 0, align 8, !dbg ![[GVE:.*]]
+@d = internal global %struct.anon zeroinitializer, align 8, !dbg !0
+
+; Function Attrs: noinline nounwind uwtable
+define dso_local i32 @e() #0 !dbg !18 {
+entry:
+  %0 = load i64, i64* getelementptr inbounds (%struct.anon, %struct.anon* @d, i32 0, i32 0), align 8
+  %add = add nsw i64 %0, 1
+  call void @llvm.dbg.value(metadata i64 %add, metadata !24, metadata !DIExpression()), !dbg !25
+  store i64 %add, i64* getelementptr inbounds (%struct.anon, %struct.anon* @d, i32 0, i32 0), align 8
+  ret i32 undef
+}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!14, !15}
+
+; CHECK: ![[GVE]] = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "d", scope: !2, file: !3, line: 6, type: !7, isLocal: true, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !{}, globals: !{!0}, splitDebugInlining: false, nameTableKind: None)
+!3 = !DIFile(filename: "t.c", directory: "/")
+!7 = distinct !DICompositeType(tag: DW_TAG_structure_type, file: !3, line: 3, flags: DIFlagFwdDecl)
+!10 = !DIBasicType(name: "long int", size: 64, encoding: DW_ATE_signed)
+!12 = !DIDerivedType(tag: DW_TAG_typedef, name: "a", file: !3, line: 2, baseType: !13)
+!13 = distinct !DICompositeType(tag: DW_TAG_structure_type, file: !3, line: 1, elements: !{})
+!14 = !{i32 7, !"Dwarf Version", i32 4}
+!15 = !{i32 2, !"Debug Info Version", i32 3}
+!18 = distinct !DISubprogram(name: "e", scope: !3, file: !3, line: 7, type: !19, scopeLine: 7, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !{})
+!19 = !DISubroutineType(types: !{!21})
+!21 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!24 = !DILocalVariable(name: "f", scope: !18, file: !3, line: 8, type: !10)
+!25 = !DILocation(line: 0, scope: !18)