Debug Info: Allow SROAed complex floating point types to be described by constants

[aprantl]

Summary

When SROA splits up composite types and individual pieces are described by integer constants, we currently default to emitting these constant pieces as unsigned bytes regardless of the encoding of the underlying types. We don't make any attempt to find the correct encoding by finding the underlying element type from the composite type.

In the attached testcase a complex float is split into two 64-bit *integers* (because the target doesn't define a 64-bit double type).
This patch relaxes the assertion in isUnsignedDIType and matches the behavior so complex_float is treated consistent with other composite types.

I agree that trusting the IR to encode integer constants properly is questionable, but it is consistent with what we do for composite types, and apparently we're not doing any better elsewhere:

void DwarfUnit::addConstantFPValue(DIE &Die, const ConstantFP *CFP) {
  // Pass this down to addConstantValue as an unsigned bag of bits.
  addConstantValue(Die, CFP->getValueAPF().bitcastToAPInt(), true);
}

Thoughts?

[probinson]

Do we test Complex anywhere else? I'm not getting any hits in test/DebugInfo/*.

test/DebugInfo/ARM/sroa-complex.ll

16

Huh. So we see udata (0) because the zero value is considered as an integer type rather than a float type? And that's supposed to be the entire 128-bit value?
I wonder if something else weird is going on with _Complex, because on Linux I get DW_AT_location with an expression that describes only one 8-byte piece.

50

This looks funny, and I don't see a reference to !16 anywhere? Is this possibly the missing other half of the _Complex value?

[aprantl]

test/DebugInfo/ARM/sroa-complex.ll

16

No this is supposed to be the lower 64 bits of the complex 128-bit value.
In order to reproduce this you need to compile for a target that does not have a native 64-bit double data type. (E.g.: -triple=thumbv7-apple-unknown-macho but not thumbv7-apple-ios)

50

I am unsure how it could survive given that nothing is referencing it, but this is most likely the DIExpression() of the aggregate complex value before it was SROA'ed.

[probinson]

test/DebugInfo/ARM/sroa-complex.ll

16

How does the consumer know this DW_AT_const_value is only the lower half? How will it find the upper half?
I brought up the Linux behavior only because it also was not describing the upper half, which makes me think there is some deeper problem with describing Complex values. Sorry, should have made that clear, I am not expecting Linux to trigger the integer thing.

[aprantl]

test/DebugInfo/ARM/sroa-complex.ll

16

Good catch, this is due to an unrelated bug. In the IR the value is clearly described by DW_OP_bit_piece(0, 64), however, the bit_piece expression doesn't make it into the final DWARF output.

Note that describing only the lower half is correct here as the original source is:

void f(_Complex double c) { c = 0; }

If you change that to become

void f(_Complex double c) { c = 0 + 1*_Complex_I; }

you will get two bit_pieces describing real and imaginary parts. However, in this case, our DWARF backend will sub-optimally decide to emit a single-entry location list (totally different code path) because there is more than one DBG_VALUE.

[probinson]

test/DebugInfo/ARM/sroa-complex.ll

16

Why is it correct to describe only half the value of the formal parameter? It is 16 bytes wide and all of it has some location or constant value.
The body of the function assigns an integer literal to a complex variable; the integer literal is promoted to double and used for the real part, and the imaginary part is set to zero. C11 6.3.1.7 says so. Therefore there is a known constant value for all the bits of 'c' and the DWARF should describe all of that value.

[aprantl]

You're correct, it looks like there is also a bug in SROA / Local that is causing the imaginary part to be dropped:

*** IR Dump After Simplify the CFG ***
; Function Attrs: nounwind optsize
define arm_aapcscc void @f([2 x i64] %c.coerce) #0 !dbg !4 {
entry:
  %c = alloca { double, double }, align 8
  %0 = bitcast { double, double }* %c to [2 x i64]*
  store [2 x i64] %c.coerce, [2 x i64]* %0, align 8
  call void @llvm.dbg.declare(metadata { double, double }* %c, metadata !10, metadata !16), !dbg !17
  %c.realp = getelementptr inbounds { double, double }, { double, double }* %c, i32 0, i32 0, !dbg !17
  %c.imagp = getelementptr inbounds { double, double }, { double, double }* %c, i32 0, i32 1, !dbg !17
  store double 0.000000e+00, double* %c.realp, align 8, !dbg !17
  store double 0.000000e+00, double* %c.imagp, align 8, !dbg !17
  ret void, !dbg !17
}
*** IR Dump After SROA ***
; Function Attrs: nounwind optsize
define arm_aapcscc void @f([2 x i64] %c.coerce) #0 !dbg !4 {
entry:
  %c.coerce.fca.0.extract = extractvalue [2 x i64] %c.coerce, 0
  call void @llvm.dbg.value(metadata i64 %c.coerce.fca.0.extract, i64 0, metadata !10, metadata !16), !dbg !17
  %c.coerce.fca.1.extract = extractvalue [2 x i64] %c.coerce, 1
  call void @llvm.dbg.value(metadata i64 %c.coerce.fca.1.extract, i64 0, metadata !10, metadata !18), !dbg !17
  call void @llvm.dbg.declare(metadata { double, double }* undef, metadata !10, metadata !19), !dbg !17
  call void @llvm.dbg.value(metadata i64 0, i64 0, metadata !10, metadata !16), !dbg !17
  ret void, !dbg !17
}

[probinson]

The basic idea of handling DWARF complex_float the same as float seems okay, but the test isn't going to look right until _Complex in general works right. I'd say it's Eric's call whether you need to shave that yak first.

[aprantl]

I found the bug causing the imaginary part to be dropped. LdStHasDebugValue in Local.cpp doesn't take DIExpressions into account, so it thinks the variable is already fully described by the dbg.value describing the lower part.
Working on a patch.

[aprantl]

Let's consider ths yak shaved:

[aprantl]

commit 9114a54cc7bd33e850ef8e2eb97f6c41880737c1
Author: Adrian Prantl <[e-mail]>
Date: Wed Feb 17 12:05:27 2016 -0800

Debug Info: Teach LdStHasDebugValue() (Local.cpp) about DIExpressions.
This function is used to check whether a dbg.value intrinsic has already
been inserted, but without comparing the DIExpression, it would erroneously
fire on split aggregates and only the first scalar would survive.

Found via http://reviews.llvm.org/D16867.
<rdar://problem/24456528>

[aprantl]

I also fixed the other bug in r261168:

DwarfDebug: Don't drop the DIExpression just because a variable is
described by an immediate.

which makes this patch obsolete. (This code path can no longer be triggered).

Thanks for all the feedback!

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

static bool isUnsignedDIType(DwarfDebug *DD, const DIType *Ty) {
   }
 
   auto *BTy = cast<DIBasicType>(Ty);
   unsigned Encoding = BTy->getEncoding();
   assert((Encoding == dwarf::DW_ATE_unsigned ||
           Encoding == dwarf::DW_ATE_unsigned_char ||
           Encoding == dwarf::DW_ATE_signed ||
           Encoding == dwarf::DW_ATE_signed_char ||
-          Encoding == dwarf::DW_ATE_float || Encoding == dwarf::DW_ATE_UTF ||
-          Encoding == dwarf::DW_ATE_boolean ||
+          Encoding == dwarf::DW_ATE_float ||
+          Encoding == dwarf::DW_ATE_complex_float ||
+          Encoding == dwarf::DW_ATE_UTF || Encoding == dwarf::DW_ATE_boolean ||
           (Ty->getTag() == dwarf::DW_TAG_unspecified_type &&
            Ty->getName() == "decltype(nullptr)")) &&
          "Unsupported encoding");
-  return Encoding == dwarf::DW_ATE_unsigned ||
+  return Encoding == dwarf::DW_ATE_float ||
+         Encoding == dwarf::DW_ATE_complex_float ||
+         Encoding == dwarf::DW_ATE_unsigned ||
          Encoding == dwarf::DW_ATE_unsigned_char ||
          Encoding == dwarf::DW_ATE_UTF || Encoding == dwarf::DW_ATE_boolean ||
          Ty->getTag() == dwarf::DW_TAG_unspecified_type;
 }
 
 /// If this type is derived from a base type then return base type size.
 static uint64_t getBaseTypeSize(DwarfDebug *DD, const DIDerivedType *Ty) {
   unsigned Tag = Ty->getTag();

test/DebugInfo/ARM/sroa-complex.ll

+; RUN: %llc_dwarf -O0 -filetype=obj -o %t.o %s
+; RUN: llvm-dwarfdump -debug-dump=info %t.o | FileCheck %s
+; REQUIRES: object-emission
+target datalayout = "e-m:o-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "thumbv7-apple-unknown-macho"
+
+; generated from (-triple thumbv7-apple-unknown-macho -Os):
+;   void f(_Complex double c) { c = 0; }
+
+; Function Attrs: norecurse nounwind optsize readnone
+define arm_aapcscc void @f([2 x i64] %c.coerce) #0 !dbg !4 {
+entry:
+  ; The target has no native double type.
+  ; SROA split the complex value into two i64 values.
+  ; CHECK: DW_TAG_formal_parameter
+  ; CHECK-NEXT:  DW_AT_const_value [DW_FORM_udata]  (0)
+  tail call void @llvm.dbg.value(metadata i64 0, i64 0, metadata !10, metadata !18), !dbg !17
+  ret void, !dbg !17
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #1
+
+attributes #0 = { norecurse nounwind optsize readnone }
+attributes #1 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!11, !12, !13, !14}
+!llvm.ident = !{!15}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 3.9.0 (trunk 259428)", isOptimized: true, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3)
+!1 = !DIFile(filename: "<stdin>", directory: "/Volumes/Data/radar/24456528")
+!2 = !{}
+!3 = !{!4}
+!4 = distinct !DISubprogram(name: "f", scope: !5, file: !5, line: 1, type: !6, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: true, variables: !9)
+!5 = !DIFile(filename: "test.c", directory: "/Volumes/Data/radar/24456528")
+!6 = !DISubroutineType(types: !7)
+!7 = !{null, !8}
+!8 = !DIBasicType(name: "complex", size: 128, align: 64, encoding: DW_ATE_complex_float)
+!9 = !{!10}
+!10 = !DILocalVariable(name: "c", arg: 1, scope: !4, file: !5, line: 1, type: !8)
+!11 = !{i32 2, !"Dwarf Version", i32 4}
+!12 = !{i32 2, !"Debug Info Version", i32 3}
+!13 = !{i32 1, !"wchar_size", i32 4}
+!14 = !{i32 1, !"min_enum_size", i32 4}
+!15 = !{!"clang version 3.9.0 (trunk 259428)"}
+!16 = !DIExpression()
+!17 = !DILocation(line: 1, scope: !4)
+!18 = !DIExpression(DW_OP_bit_piece, 0, 64)

test/DebugInfo/X86/float_const.ll

 ; RUN: llc < %s -filetype=obj | llvm-dwarfdump -debug-dump=info - | FileCheck %s
 ; from (at -Os):
 ; void foo() {
 ;   float a = 3.14;
 ;   *(int *)&a = 0;
 ; }
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.10.0"
 
 ; Function Attrs: nounwind optsize readnone uwtable
 define void @foo() #0 !dbg !7 {
 entry:
   tail call void @llvm.dbg.declare(metadata float* undef, metadata !13, metadata !19), !dbg !20
   tail call void @llvm.dbg.value(metadata i32 1078523331, i64 0, metadata !13, metadata !19), !dbg !20
   tail call void @llvm.dbg.value(metadata i32 0, i64 0, metadata !13, metadata !19), !dbg !20
-; CHECK:  DW_AT_const_value [DW_FORM_sdata]    (0)
+; CHECK:  DW_AT_const_value [DW_FORM_udata]    (0)
 ; CHECK-NEXT: DW_AT_name {{.*}}"a"
   ret void, !dbg !21
 }
 
 ; Function Attrs: nounwind readnone
 declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
 
 ; Function Attrs: nounwind readnone