[DebugInfo] Refactor code for emitting DWARF expressions for FP constants

[labath]

Summary

This patch moves the selection of the style used to emit the numbers
(DW_OP_implicit_value vs. DW_OP_const+DW_OP_stack_value) into
DwarfExpression::addUnsignedConstant. This logic is not FP-specific, and
it will be needed for large integers too.

Split off from D90916.

[dblaikie]

Looks good

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

234–236

This looks like a change in behavior - is it? Should it be tested?

247

This looks like a change in behavior (now other <= 8 floating point sizes can be emitted - is it just that other such sizes are impossible/never come up? In which case maybe an assertion would be suitable?) & might possibly benefit from testing.

[labath]

Add a _Float16 test

[labath]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

234–236

It shouldn't normally be a change because this adjustment in DW_OP_stack_value emission is matched by a change in the callers (previously it was emitted through the DwarfExpr.addExpression(std::move(ExprCursor)) call, now that call is skipped).

It could theoretically make a difference if we had a floating point constant accompanied by a non-empty dwarf expression -- previously that expression would be emitted, but now it's skipped. However, such a combination wouldn't be very useful (the expression would operate on the constant as an integer), and judging by the lack of failing tests, we don't actually make use of it. (Though my knowledge of this part of the code is fairly limited).

247

Actually, it was already possible to emit the DW_OP_stack_value flavour of _Float16 (for example). This added the ability to handle the DW_OP_implicit_value form as well, because the new code layout made it harder to split the two cases. I didn't want to add a test without checking that this actually works (though I had no reason to suspect it wouldn't), but I have now verified that both gdb and lldb handle _Float16 (on arm), so I am adding a test for it.

[labath]

• reduce the test

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

247

Cool, thanks!

[dyung]

Hi Pavel, your change caused an assertion failure to be hit in our internal test suite. I have filed a repro with the assertion failure and a callstack in PR48277. Can you please take a look?

[mstorsjo]

I also ran into this issue (and added a second repro to the bug report); I'd suggest a revert, but I think @labath may get to it fairly soon, so holding off of reverting until the end of the day.

[labath]

I reverted this because it caused issues with complex floats. See PR48277 and D92013 for details.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

if (Value.isInt()) {
     if (!DwarfExpr.addMachineRegExpression(TRI, Cursor, Location.getReg()))
       return;
     return DwarfExpr.addExpression(std::move(Cursor));
   } else if (Value.isTargetIndexLocation()) {
     TargetIndexLocation Loc = Value.getTargetIndexLocation();
     // TODO TargetIndexLocation is a target-independent. Currently only the WebAssembly-specific
     // encoding is supported.
     DwarfExpr.addWasmLocation(Loc.Index, static_cast<uint64_t>(Loc.Offset));
-      DwarfExpr.addExpression(std::move(ExprCursor));
-      return;
   } else if (Value.isConstantFP()) {
-    if (AP.getDwarfVersion() >= 4 && !AP.getDwarfDebug()->tuneForSCE()) {
-      DwarfExpr.addConstantFP(Value.getConstantFP()->getValueAPF(), AP);
-      return;
-    } else if (Value.getConstantFP()
-                   ->getValueAPF()
-                   .bitcastToAPInt()
-                   .getBitWidth() <= 64 /*bits*/)
-      DwarfExpr.addUnsignedConstant(
-          Value.getConstantFP()->getValueAPF().bitcastToAPInt());
-    else
-      LLVM_DEBUG(
-          dbgs()
-          << "Skipped DwarfExpression creation for ConstantFP of size"
-          << Value.getConstantFP()->getValueAPF().bitcastToAPInt().getBitWidth()
-          << " bits\n");
   }
   DwarfExpr.addExpression(std::move(ExprCursor));
 }
 
 void DebugLocEntry::finalize(const AsmPrinter &AP,
                              DebugLocStream::ListBuilder &List,
                              const DIBasicType *BT,
                              DwarfCompileUnit &TheCU) {

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h

public:
 
   /// Emit a signed constant.
   void addSignedConstant(int64_t Value);
 
   /// Emit an unsigned constant.
   void addUnsignedConstant(uint64_t Value);
 
   /// Emit an unsigned constant.
-  void addUnsignedConstant(const APInt &Value);
+  void addUnsignedConstant(APInt Value, const AsmPrinter &AP);
 
   /// Emit an floating point constant.
   void addConstantFP(const APFloat &Value, const AsmPrinter &AP);
 
   /// Lock this down to become a memory location description.
   void setMemoryLocationKind() {
     assert(isUnknownLocation());
     LocationKind = Memory;

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

 }
 
 void DwarfExpression::addUnsignedConstant(uint64_t Value) {
   assert(isImplicitLocation() || isUnknownLocation());
   LocationKind = Implicit;
   emitConstu(Value);
 }
 
-void DwarfExpression::addUnsignedConstant(const APInt &Value) {
+void DwarfExpression::addUnsignedConstant(APInt Value, const AsmPrinter &AP) {
   assert(isImplicitLocation() || isUnknownLocation());
   LocationKind = Implicit;
 
+  if (AP.getDwarfVersion() >= 4 && !AP.getDwarfDebug()->tuneForSCE()) {
+    int NumBytes = Value.getBitWidth() / 8;
+    emitOp(dwarf::DW_OP_implicit_value);
+    emitUnsigned(NumBytes /*Size of the block in bytes*/);
+
+    // The loop below is emitting the value starting at least significant
+    // byte, so we need to perform a byte-swap to get the byte order correct
+    // in case of a big-endian target.
+    if (AP.getDataLayout().isBigEndian())
+      Value = Value.byteSwap();
+
+    for (int i = 0; i < NumBytes; ++i) {
+      emitData1(Value.getRawData()[0] & 0xFF);
+      Value = Value.lshr(8);
+    }
+
+    return;
+  }
+
   unsigned Size = Value.getBitWidth();
   const uint64_t *Data = Value.getRawData();
 
   // Chop it up into 64-bit pieces, because that's the maximum that
   // addUnsignedConstant takes.
   unsigned Offset = 0;
   while (Offset < Size) {
     addUnsignedConstant(*Data++);
+    addStackValue();
     if (Offset == 0 && Size <= 64)
       break;
-    addStackValue();
     addOpPiece(std::min(Size - Offset, 64u), Offset);
     Offset += 64;
   }
 }
 
 void DwarfExpression::addConstantFP(const APFloat &APF, const AsmPrinter &AP) {
   assert(isImplicitLocation() || isUnknownLocation());
   APInt API = APF.bitcastToAPInt();
   int NumBytes = API.getBitWidth() / 8;
-  if (NumBytes == 4 /*float*/ || NumBytes == 8 /*double*/) {
-    // FIXME: Add support for `long double`.
-    emitOp(dwarf::DW_OP_implicit_value);
-    emitUnsigned(NumBytes /*Size of the block in bytes*/);
-
-    // The loop below is emitting the value starting at least significant byte,
-    // so we need to perform a byte-swap to get the byte order correct in case
-    // of a big-endian target.
-    if (AP.getDataLayout().isBigEndian())
-      API = API.byteSwap();
-
-    for (int i = 0; i < NumBytes; ++i) {
-      emitData1(API.getZExtValue() & 0xFF);
-      API = API.lshr(8);
-    }
-
-    return;
-  }
+  if (NumBytes <= 8 /*double*/)
+    addUnsignedConstant(API, AP);
+  else
     LLVM_DEBUG(
-      dbgs() << "Skipped DW_OP_implicit_value creation for ConstantFP of size: "
+        dbgs() << "Skipped DwarfExpression creation for ConstantFP of size"
                << API.getBitWidth() << " bits\n");
 }
 
 bool DwarfExpression::addMachineRegExpression(const TargetRegisterInfo &TRI,
                                               DIExpressionCursor &ExprCursor,
                                               llvm::Register MachineReg,
                                               unsigned FragmentOffsetInBits) {
   auto Fragment = ExprCursor.getFragmentInfo();
   if (!addMachineReg(TRI, MachineReg, Fragment ? Fragment->SizeInBits : ~1U)) {

llvm/test/DebugInfo/ARM/implicit_value-_Float16.ll

+; RUN: llc -debugger-tune=gdb -filetype=obj %s -o - | llvm-dwarfdump - \
+; RUN:   | FileCheck %s --check-prefixes=GDB,BOTH
+; RUN: llc -debugger-tune=sce -filetype=obj %s -o - | llvm-dwarfdump - \
+; RUN:   | FileCheck %s --check-prefixes=SCE,BOTH
+
+; BOTH: DW_TAG_variable
+; BOTH-NEXT:  DW_AT_location
+; GDB-NEXT:      {{.*}}: DW_OP_implicit_value 0x2 0xe0 0x51
+; SCE-NEXT:      {{.*}}: DW_OP_constu 0x51e0, DW_OP_stack_value
+; GDB-NEXT:      {{.*}}: DW_OP_implicit_value 0x2 0x40 0x51
+; SCE-NEXT:      {{.*}}: DW_OP_constu 0x5140, DW_OP_stack_value
+; BOTH-NEXT:  DW_AT_name    ("a")
+
+source_filename = "-"
+target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "armv7-unknown-unknown"
+
+define dso_local arm_aapcscc void @f() local_unnamed_addr !dbg !8 {
+entry:
+  call void @llvm.dbg.value(metadata half 0xH51E0, metadata !13, metadata !DIExpression()), !dbg !15
+  call arm_aapcscc void bitcast (void (...)* @g to void ()*)(), !dbg !15
+  call void @llvm.dbg.value(metadata half 0xH5140, metadata !13, metadata !DIExpression()), !dbg !15
+  call arm_aapcscc void bitcast (void (...)* @g to void ()*)(), !dbg !15
+  ret void, !dbg !15
+}
+
+declare dso_local arm_aapcscc void @g(...) local_unnamed_addr
+
+declare void @llvm.dbg.value(metadata, metadata, metadata) #2
+
+attributes #2 = { nofree nosync nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "-", directory: "/")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{i32 1, !"min_enum_size", i32 4}
+!7 = !{!"clang version 12.0.0"}
+!8 = distinct !DISubprogram(name: "f", scope: !1, file: !1, type: !10, unit: !0)
+!10 = !DISubroutineType(types: !11)
+!11 = !{null}
+!12 = !{!13}
+!13 = !DILocalVariable(name: "a", scope: !8, file: !1, type: !14)
+!14 = !DIBasicType(name: "_Float16", size: 16, encoding: DW_ATE_float)
+!15 = !DILocation(line: 0, scope: !8)