[DebugInfo] Print empty MDTuples wrapped in MetadataAsValue inline

[Orlando]

Summary

This improves the readability of debugging intrinsics. Instead of:

call void @llvm.dbg.value(metadata !2, ...)
!2 = !{}

We will see:

call void @llvm.dbg.value(metadata !{}, ...)
!2 = !{}

Note that we still get a numbered metadata entry for the node even if it's not
used elsewhere. This is to avoid adding more context to the print functions.

This is already legal IR - LLVM can parse and understand it - so there is no
need to update the parser.

The next patches in this stack will make such empty metadata operands more
common and semantically important.

Related to https://discourse.llvm.org/t/auto-undef-debug-uses-of-a-deleted-value

[StephenTozer]

Largely LGTM, but with a potential error that needs looking at.

llvm/lib/IR/AsmWriter.cpp

4884–4885

Setting this to false, while it makes total sense to me, caused an error in the LLDB test suite last time it was done[0][1]. Off the top of my head I'm not sure under what circumstances exactly we're calling printMetadataImpl directly for a LocalAsMetadata or DIArgList, but (much as a pain as it is) it probably needs to be verified that this error won't come up again - happy to investigate further myself if needed.

[0] https://reviews.llvm.org/D104827#2856347
[1] https://reviews.llvm.org/D104827#2898216

[jmorse]

I agree with Stephen, it's good but flipping the FromValue flag doesn't seem right.

llvm/lib/IR/AsmWriter.cpp

4884–4885

Yeah, changing this feels at risk of being overkill -- I can understand why we want the condition that FromValue is true when printing inline, but why does this call-site need to change for that? Possibly printMetadataImpl needs to take a FromValue argument itself and push the decision making higher up the call stack?

llvm/test/DebugInfo/Generic/empty-metadata-roundtrip.ll

7

Worth using the term "cosmetic" to fully communicate that there's no functional impact of this behaviour?

[Orlando]

Rather than using FromValue to determine whether the node should be printed inline I've added a parameter AsOperand to WriteAsOperandInternal, which is true when printing metadata with Metadata::printAsOperand and when printing MetadataAsValue wrapped operands through the Value * version of WriteAsOperandInternal.

[Orlando]

+ Update test comment.

[StephenTozer]

Changes LGTM.

[thakis]

This breaks check-clang, eg http://45.33.8.238/linux/105304/step_7.txt

Please take a look and revert for now if it takes a while to fix.

[Orlando]

Thanks for the heads up @thakis. Looks like there are simply some more tests that need updating to the new formatting, but I've reverted for now as it tripped several bots.

[Orlando]

This patch prints the tuple as !{} even if it's distinct, which I thought was reasonable when writing the patch. If a distinct empty tuple is used multiple times or is attached to multiple instructions however it might be useful to have the metadata ID printed, to make debugging easier.

(example bot failure with distinct empty metadata: In https://lab.llvm.org/buildbot/#/builders/245/builds/7602/steps/5/logs/FAIL__Clang__type-metadata_cpp - the distinct MD is not used in multiple places in this case but it seems like a possibility)

With that in mind I think we should either further restrict this to non-distinct empty MD tuples (IMO still useful) or drop this patch (just continue to use metadata references, so !2 inline and !2 = !{} (+-distinct) in the metadata list).

Does anyone have any strong feelings? @jmorse, @StephenTozer ?

If not, then, although I do prefer the inline printing aesthetically, I think that the extra code isn't worth it for the slight readability improvement (and test-writing convenience of not needing to check the metadata reference), and vote that we drop this.

[Orlando]

Abandoning this (see previous comment)

llvm/lib/IR/AsmWriter.cpp

 // AsmWriter Implementation
 //===----------------------------------------------------------------------===//
 
 static void WriteAsOperandInternal(raw_ostream &Out, const Value *V,
                                    AsmWriterContext &WriterCtx);
 
 static void WriteAsOperandInternal(raw_ostream &Out, const Metadata *MD,
                                    AsmWriterContext &WriterCtx,
-                                   bool FromValue = false);
+                                   bool FromValue = false,
+                                   bool AsOperand = false);
 
 static void WriteOptimizationInfo(raw_ostream &Out, const User *U) {
   if (const FPMathOperator *FPO = dyn_cast<const FPMathOperator>(U))
     Out << FPO->getFastMathFlags();
 
   if (const OverflowingBinaryOperator *OBO =
         dyn_cast<OverflowingBinaryOperator>(U)) {
     if (OBO->hasNoUnsignedWrap())
if (const InlineAsm *IA = dyn_cast<InlineAsm>(V)) {
     Out << "\", \"";
     printEscapedString(IA->getConstraintString(), Out);
     Out << '"';
     return;
   }
 
   if (auto *MD = dyn_cast<MetadataAsValue>(V)) {
     WriteAsOperandInternal(Out, MD->getMetadata(), WriterCtx,
-                           /* FromValue */ true);
+                           /* FromValue */ true, /* AsOperand */ true);
     return;
   }
 
   char Prefix = '%';
   int Slot;
   auto *Machine = WriterCtx.Machine;
   // If we have a SlotTracker, use it.
   if (Machine) {
static void WriteAsOperandInternal(raw_ostream &Out, const Value *V,
 
   if (Slot != -1)
     Out << Prefix << Slot;
   else
     Out << "<badref>";
 }
 
 static void WriteAsOperandInternal(raw_ostream &Out, const Metadata *MD,
-                                   AsmWriterContext &WriterCtx,
-                                   bool FromValue) {
+                                   AsmWriterContext &WriterCtx, bool FromValue,
+                                   bool AsOperand) {
   // Write DIExpressions and DIArgLists inline when used as a value. Improves
   // readability of debug info intrinsics.
   if (const DIExpression *Expr = dyn_cast<DIExpression>(MD)) {
     writeDIExpression(Out, Expr, WriterCtx);
     return;
   }
   if (const DIArgList *ArgList = dyn_cast<DIArgList>(MD)) {
     writeDIArgList(Out, ArgList, WriterCtx, FromValue);
     return;
   }
 
   if (const MDNode *N = dyn_cast<MDNode>(MD)) {
+    // Write empty metadata tuples wrapped in MetadataAsValue inline.
+    if (isa<MDTuple>(N) && !N->getNumOperands() && AsOperand) {
+      WriteMDNodeBodyInternal(Out, N, WriterCtx);
+      return;
+    }
+
     std::unique_ptr<SlotTracker> MachineStorage;
     SaveAndRestore SARMachine(WriterCtx.Machine);
     if (!WriterCtx.Machine) {
       MachineStorage = std::make_unique<SlotTracker>(WriterCtx.Context);
       WriterCtx.Machine = MachineStorage.get();
     }
     int Slot = WriterCtx.Machine->getMetadataSlot(N);
     if (Slot == -1) {
static void printMetadataImpl(raw_ostream &ROS, const Metadata &MD,
   std::unique_ptr<AsmWriterContext> WriterCtx;
   if (PrintAsTree && !OnlyAsOperand)
     WriterCtx = std::make_unique<MDTreeAsmWriterContext>(
         &TypePrinter, MST.getMachine(), M, OS, &MD);
   else
     WriterCtx =
         std::make_unique<AsmWriterContext>(&TypePrinter, MST.getMachine(), M);
 
-  WriteAsOperandInternal(OS, &MD, *WriterCtx, /* FromValue */ true);
+  WriteAsOperandInternal(OS, &MD, *WriterCtx, /* FromValue */ true,
+                         OnlyAsOperand);
 
   auto *N = dyn_cast<MDNode>(&MD);
   if (OnlyAsOperand || !N || isa<DIExpression>(MD) || isa<DIArgList>(MD))
     return;
 
   OS << " = ";
   WriteMDNodeBodyInternal(OS, N, *WriterCtx);
 }

llvm/test/DebugInfo/Generic/empty-metadata-roundtrip.ll

+; RUN: opt -passes=verify %s -o - | opt -S -o - | FileCheck %s
+
+;; Check that an empty metadata node is printed inline when wrapped in
+;; MetadataAsValue.
+;; We still get a numbered metadata entry for the node even though it's not
+;; used elsewhere. This is a purely cosmetic result of reducing the complexity
+;; of the printing functions; it's not a requirement so it is okay to update
+;; this part of the test if that changes in the future.
+
+; CHECK: call void @llvm.dbg.declare(metadata !{},
+; CHECK: ![[#]] = !{}
+
+define dso_local void @fun() local_unnamed_addr #0 !dbg !9 {
+entry:
+  call void @llvm.dbg.declare(metadata !{}, metadata !13, metadata !DIExpression()), !dbg !15
+  ret void, !dbg !16
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6, !7}
+!llvm.ident = !{!8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 16.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "/")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 8, !"PIC Level", i32 2}
+!6 = !{i32 7, !"PIE Level", i32 2}
+!7 = !{i32 7, !"uwtable", i32 2}
+!8 = !{!"clang version 16.0.0"}
+!9 = distinct !DISubprogram(name: "fun", scope: !1, file: !1, line: 1, type: !10, scopeLine: 1, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
+!10 = !DISubroutineType(types: !11)
+!11 = !{null}
+!12 = !{!13}
+!13 = !DILocalVariable(name: "a", scope: !9, file: !1, line: 1, type: !14)
+!14 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!15 = !DILocation(line: 1, column: 18, scope: !9)
+!16 = !DILocation(line: 1, column: 21, scope: !9)
+

llvm/test/Transforms/GlobalOpt/localize-constexpr-debuginfo.ll

 ; RUN: opt -S < %s -passes=globalopt | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 @_ZL1x = internal global ptr null, align 8, !dbg !0
 
 define i32 @main(i32 %argc, ptr %argv) norecurse !dbg !18 {
 ; CHECK: define i32 @main
 ; Make sure we localized the global.
 ; CHECK: alloca ptr
 ; Make sure the metadata is sane. Currently, we just drop the metadata,
 ; so it points to nothing.
-; CHECK: call void @llvm.dbg.value(metadata !2,
-; CHECK: !2 = !{}
+; CHECK: call void @llvm.dbg.value(metadata !{},
+
 entry:
   call void @llvm.dbg.value(metadata i32 %argc, metadata !22, metadata !23), !dbg !24
   call void @llvm.dbg.value(metadata ptr %argv, metadata !25, metadata !23), !dbg !26
   %0 = load ptr, ptr %argv, align 8, !dbg !27
   store ptr %0, ptr @_ZL1x, align 8, !dbg !29
   call void @llvm.dbg.value(metadata ptr @_ZL1x, metadata !30, metadata !23), !dbg !31
   %1 = load ptr, ptr @_ZL1x, align 8, !dbg !32
   %2 = load i8, ptr %1, align 1, !dbg !33

llvm/test/Transforms/GlobalOpt/metadata.ll

 }
 
 define void @foo(i32 %x) {
 ; Note: these arguments look like MDNodes, but they're really syntactic sugar
 ; for 'MetadataAsValue::get(ValueAsMetadata::get(Value*))'.  When @G drops to
 ; null, the ValueAsMetadata instance gets replaced by metadata !{}, or
 ; MDNode::get({}).
   call void @llvm.foo(metadata ptr @G, metadata i32 %x)
-; CHECK: call void @llvm.foo(metadata ![[EMPTY:[0-9]+]], metadata i32 %x)
+; CHECK: call void @llvm.foo(metadata !{}, metadata i32 %x)
   ret void
 }
 
 declare void @llvm.foo(metadata, metadata) nounwind readnone
 
 !named = !{!0}
 ; CHECK: !named = !{![[NULL:[0-9]+]]}
 
 !0 = !{ptr @G}
 ; CHECK-DAG: ![[NULL]] = distinct !{null}
-; CHECK-DAG: ![[EMPTY]] = !{}

llvm/test/Transforms/LoopIdiom/debug-line.ll

 ; CHECK-NEXT:    br label [[FOR_BODY:%.*]]
 ; CHECK:       for.body:
 ; CHECK-NEXT:    [[INDVAR:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[INDVAR_NEXT:%.*]], [[FOR_BODY]] ]
 ; CHECK-NEXT:    [[ARRAYIDX:%.*]] = getelementptr double, ptr [[A]], i64 [[INDVAR]]
 ; CHECK-NEXT:    [[INDVAR_NEXT]] = add i64 [[INDVAR]], 1
 ; CHECK-NEXT:    [[EXITCOND:%.*]] = icmp ne i64 [[INDVAR_NEXT]], 1000
 ; CHECK-NEXT:    br i1 [[EXITCOND]], label [[FOR_BODY]], label [[FOR_END:%.*]], !dbg [[DBG15]]
 ; CHECK:       for.end:
-; CHECK-NEXT:    tail call void @llvm.dbg.value(metadata [[META3:![0-9]+]], metadata [[META11]], metadata !DIExpression()), !dbg [[DBG17:![0-9]+]]
+; CHECK-NEXT:    tail call void @llvm.dbg.value(metadata !{}, metadata [[META11]], metadata !DIExpression()), !dbg [[DBG17:![0-9]+]]
 ; CHECK-NEXT:    ret void, !dbg [[DBG18:![0-9]+]]
 ;
 entry:
   tail call void @llvm.dbg.value(metadata ptr %a, metadata !5, metadata !DIExpression()), !dbg !8
   tail call void @llvm.dbg.value(metadata i32 0, metadata !10, metadata !DIExpression()), !dbg !14
   br label %for.body
 
 for.body:                                         ; preds = %entry, %for.body

llvm/unittests/IR/MetadataTest.cpp

TEST_F(MDNodeTest, PrintFromMetadataAsValue) {
   auto *N1 = MDNode::getDistinct(Context, std::nullopt);
   auto *MAV0 = MetadataAsValue::get(Context, N0);
   auto *MAV1 = MetadataAsValue::get(Context, N1);
   CallInst::Create(Intrinsic, MAV0, "", BB0);
   CallInst::Create(Intrinsic, MAV1, "", BB1);
 
   EXPECT_PRINTER_EQ("!0 = distinct !{}", MAV0->print(OS));
   EXPECT_PRINTER_EQ("!1 = distinct !{}", MAV1->print(OS));
-  EXPECT_PRINTER_EQ("!0", MAV0->printAsOperand(OS, false));
-  EXPECT_PRINTER_EQ("!1", MAV1->printAsOperand(OS, false));
-  EXPECT_PRINTER_EQ("metadata !0", MAV0->printAsOperand(OS, true));
-  EXPECT_PRINTER_EQ("metadata !1", MAV1->printAsOperand(OS, true));
+  EXPECT_PRINTER_EQ("distinct !{}", MAV0->printAsOperand(OS, false));
+  EXPECT_PRINTER_EQ("distinct !{}", MAV1->printAsOperand(OS, false));
+  EXPECT_PRINTER_EQ("metadata distinct !{}", MAV0->printAsOperand(OS, true));
+  EXPECT_PRINTER_EQ("metadata distinct !{}", MAV1->printAsOperand(OS, true));
 
   ModuleSlotTracker MST(&M);
   EXPECT_PRINTER_EQ("!0 = distinct !{}", MAV0->print(OS, MST));
   EXPECT_PRINTER_EQ("!1 = distinct !{}", MAV1->print(OS, MST));
-  EXPECT_PRINTER_EQ("!0", MAV0->printAsOperand(OS, false, MST));
-  EXPECT_PRINTER_EQ("!1", MAV1->printAsOperand(OS, false, MST));
-  EXPECT_PRINTER_EQ("metadata !0", MAV0->printAsOperand(OS, true, MST));
-  EXPECT_PRINTER_EQ("metadata !1", MAV1->printAsOperand(OS, true, MST));
+  EXPECT_PRINTER_EQ("distinct !{}", MAV0->printAsOperand(OS, false, MST));
+  EXPECT_PRINTER_EQ("distinct !{}", MAV1->printAsOperand(OS, false, MST));
+  EXPECT_PRINTER_EQ("metadata distinct !{}",
+                    MAV0->printAsOperand(OS, true, MST));
+  EXPECT_PRINTER_EQ("metadata distinct !{}",
+                    MAV1->printAsOperand(OS, true, MST));
 }
 
 TEST_F(MDNodeTest, PrintWithDroppedCallOperand) {
   Module M("test", Context);
 
   auto *FTy = FunctionType::get(Type::getVoidTy(Context), false);
   auto *F0 = Function::Create(FTy, GlobalValue::ExternalLinkage, "F0", &M);
   auto *F1 = Function::Create(FTy, GlobalValue::ExternalLinkage, "F1", &M);