[Transforms] Fix a wrong debug info intrinsic call in `mem2reg` pass for ref 128-bit

[slydiman]

Summary

Calling device function with reference to __int128_t parameters emits incorrect debug info intrinsic call.
This happens in the mem2reg pass, while eliminating allocas it also tries to update the debug intrinsics.
For a reference parameter, the fragment size is the size of the underlying object. When eliminating alloca for double pointer to the object with a pointer to another object, the code in question wrongly assumes that a pointer doesn't fully cover the fragment.

This patch fixes that and adds a new test to cover this case.

[tra]

I'm not familiar with debug info at all.
@dblaikie -- does this make sense to you? If it matters, i128 in NVPTX is not a native register type and is passed around as a pair of i64.

[jmorse]

While the patch does fix the immediate issue, I'm not certain that disabling the valueCoversEntireFragment check in the context of pointers is a general solution. Otherwise generic code that does something strange (like breaking up allocas) would expect dbg.value's to become undef, and not doing that for certain types risks causing unexpected behaviour in the future. It's also good practice to be conservative in what input we accept here.

It seems that the "size" field of DIDerivedType nodes is optional -- I'm not sure why. Regardless, by adding "size: 64" to !10 in your test, an unpatched opt produces the correct output. Are you working from a source-language reproducer, and wouldn't the more important bug be whatever caused the DIDerivedType size field to either be unassigned, or dropped?

(It's also not clear to me why LLVM has chosen to not have a DIDerivedType specific getSizeInBits field, as it would be able to identify the type size from it's tag being a pointer or reference. However, it looks like the existing design expects the size to be provided in the "size" field, so we should try fixing that first).

[slydiman]

I have simplified the test and made it generic.

I tried to update the size of DIDerivedType in llvm/lib/AsmParser/LLParser.cpp:

bool LLParser::parseDIDerivedType(MDNode *&Result, bool IsDistinct) {
  ...

  if (tag.Val == dwarf::DW_TAG_reference_type && size.Val == 0)
    size = M->getDataLayout().getPointerSizeInBits();

  Result = GET_OR_DISTINCT(DIDerivedType,
                           (Context, tag.Val, name.Val, file.Val, line.Val,
                            scope.Val, baseType.Val, size.Val, align.Val,
                            offset.Val, DWARFAddressSpace, flags.Val,
                            extraData.Val, annotations.Val));
  return false;
}

It solved the problem too, but it caused the appearance of this field size for DIDerivedType in the output:
!10 = !DIDerivedType(tag: DW_TAG_reference_type, baseType: !4) -> !10 = !DIDerivedType(tag: DW_TAG_reference_type, baseType: !4, size: 64)

[slydiman]

Ping

[Orlando]

Hi @slydiman,

It looks like the clang front end emits a size field for reference types. e.g.

$ clang++ --version
clang version 15.0.0 (https://github.com/llvm/llvm-project.git 8e9528cb544a3d70cea96f1f99318643095a1410)
Target: x86_64-unknown-linux-gnu

$ cat test.cpp
void fun(__int128_t &i) {}

$ clang++ test.cpp -Xclang -disable-llvm-passes -g -emit-llvm -S -o -
...
!11 = !DIDerivedType(tag: DW_TAG_reference_type, baseType: !12, size: 64)
!12 = !DIDerivedType(tag: DW_TAG_typedef, name: "__int128_t", file: !13, baseType: !14)
!13 = !DIFile(filename: "test.cpp", directory: "/home/och/dev/bugs/scratch")
!14 = !DIBasicType(name: "__int128", size: 128, encoding: DW_ATE_signed)
!15 = !{}
!16 = !DILocalVariable(name: "i", arg: 1, scope: !8, file: !1, line: 1, type: !11)

Notice that i's (!16) type is !11 which is a DW_TAG_reference_type type DIDerivedType with a field size: 64. IIUC this means valueCoversEntireFragment should work correctly for this type without your patch.

Perhaps this can be fixed by patching your front end. Was your reproducer created using the clang front end?

llvm/lib/Transforms/Utils/Local.cpp

 /// ConvertDebugDeclareToDebugValue functions. The dbg.declare/dbg.addr that is
 /// converted describes an alloca'd variable, so we need to use the
 /// alloc size of the value when doing the comparison. E.g. an i1 value will be
 /// identified as covering an n-bit fragment, if the store size of i1 is at
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout &DL = DII->getModule()->getDataLayout();
   TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+
+  // If we are replacing a double indirection with a single-indirection, the
+  // fragment size could be (happens with reference params) that of the object
+  // but the pointer will only be 32/64 bits.
+  if (ValTy->isPointerTy())
+    return true;
+
   if (Optional<uint64_t> FragmentSize = DII->getFragmentSizeInBits()) {
     assert(!ValueSize.isScalable() &&
            "Fragments don't work on scalable types.");
     return ValueSize.getFixedSize() >= *FragmentSize;
   }
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.

llvm/test/Transforms/Util/nvptx-int128-ref-debuginfo.ll

+; RUN: opt -mem2reg -S < %s | FileCheck %s
+
+source_filename = "moduleOutput"
+target datalayout = "e-p:64:64:64-p3:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-i128:128:128-f32:32:32-f64:64:64-v16:16:16-v32:32:32-v64:64:64-v128:128:128-n16:32:64"
+target triple = "nvptx64-nvidia-cuda"
+
+; CHECK-LABEL: @_Z3sumRnn
+define i128 @_Z3sumRnn(i128* %foo, i128 %bar) !dbg !7 {
+  %retval = alloca i128, align 16
+  %foo.addr = alloca i128*, align 8
+  %bar.addr = alloca i128, align 16
+  store i128* %foo, i128** %foo.addr, align 8
+; CHECK: call void @llvm.dbg.value(metadata i128* %foo, metadata !11, metadata !DIExpression()), !dbg !12
+  call void @llvm.dbg.declare(metadata i128** %foo.addr, metadata !11, metadata !DIExpression()), !dbg !12
+  %1 = bitcast i128* %bar.addr to <2 x i64>*
+  %2 = bitcast i128 %bar to <2 x i64>
+  store <2 x i64> %2, <2 x i64>* %1, align 16
+; CHECK: call void @llvm.dbg.declare(metadata i128* %bar.addr, metadata !13, metadata !DIExpression()), !dbg !14
+  call void @llvm.dbg.declare(metadata i128* %bar.addr, metadata !13, metadata !DIExpression()), !dbg !14
+  %tmp = load i128*, i128** %foo.addr, align 8, !dbg !15
+  %3 = bitcast i128* %tmp to <2 x i64>*, !dbg !15
+  %4 = load <2 x i64>, <2 x i64>* %3, align 16, !dbg !15
+  %5 = bitcast <2 x i64> %4 to i128, !dbg !15
+  %tmp1 = load i128, i128* %tmp, align 16, !dbg !15
+  %6 = bitcast i128* %bar.addr to <2 x i64>*, !dbg !15
+  %7 = load <2 x i64>, <2 x i64>* %6, align 16, !dbg !15
+  %8 = bitcast <2 x i64> %7 to i128, !dbg !15
+  %tmp2 = load i128, i128* %bar.addr, align 16, !dbg !15
+  %add = add nsw i128 %5, %8, !dbg !15
+  %9 = bitcast i128* %retval to <2 x i64>*, !dbg !15
+  %10 = bitcast i128 %add to <2 x i64>, !dbg !15
+  store <2 x i64> %10, <2 x i64>* %9, align 16, !dbg !15
+  %11 = bitcast i128* %retval to <2 x i64>*, !dbg !15
+  %12 = load <2 x i64>, <2 x i64>* %11, align 16, !dbg !15
+  %13 = bitcast <2 x i64> %12 to i128, !dbg !15
+  %14 = load i128, i128* %retval, align 16, !dbg !15
+  ret i128 %13, !dbg !15
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #0
+
+attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "foo", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3)
+!1 = !DIFile(filename: "test.cu", directory: "/foo/bar")
+!2 = !{}
+!3 = !{!4}
+!4 = !DIDerivedType(tag: DW_TAG_typedef, name: "__int128_t", scope: !1, file: !1, baseType: !5)
+!5 = !DIBasicType(name: "__int128", size: 128, encoding: DW_ATE_signed)
+!6 = !{i32 1, !"Debug Info Version", i32 3}
+!7 = distinct !DISubprogram(name: "sum", linkageName: "_Z3sumRnn", scope: !1, file: !1, line: 5, type: !8, scopeLine: 5, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!4, !10, !4}
+!10 = !DIDerivedType(tag: DW_TAG_reference_type, baseType: !4)
+!11 = !DILocalVariable(name: "foo", arg: 1, scope: !7, file: !1, line: 5, type: !10)
+!12 = !DILocation(line: 5, column: 52, scope: !7)
+!13 = !DILocalVariable(name: "bar", arg: 2, scope: !7, file: !1, line: 5, type: !4)
+!14 = !DILocation(line: 5, column: 68, scope: !7)
+!15 = !DILocation(line: 7, column: 11, scope: !16)
+!16 = distinct !DILexicalBlock(scope: !7, file: !1, line: 6, column: 1)