Remove Debug metadata from copied instruction to prevent Module verification failure

[singam-sanjay]

Summary

Remove debug metadata from instruction to be copied to prevent the source file's debug metadata being copied into GPUModule and eventually failing Module verification and ASM string codegeneration.

When copying the instruction onto the Module meant for the GPU, debug metadata attached to an instruction causes all related metadata to be pulled into the Module, including the DICompileUnit, which is not listed in llvm.dbg.cu of the Module. This fails the verification of the Module and generation of the ASM string.

The only debug metadata of the instruction, the DebugLoc, is unset by this patch.

[grosser]

Amazing. Can you add a test case?

[grosser]

Also mark this as requesting changes to make sure this is not in my review queue any more.

[singam-sanjay]

Added test case.

[bollu]

test/GPGPU/debug-metadata-leak.ll

2

Could you make it something a little more dominant? Inst would be easier to spot, IMO.

Also, putting emphasis in that sentence may help:

The instruction marked `Inst` ...

2

Usually, I have seen ; RUN lines at the top of the file, followed by the explanation. Have you considered sticking to this order?

24

Could you please add in a comment the corresponding C source? Or a C function that performs the same operation? I find having something like that helps later on for context :). If it's too much trouble, nvm.

In this case, I believe it would be something like:

pseudocode.c

void vec_add_1(int N, int *arr) {
   for(int index = 0; index < N; index += 8) {
      %3 = {undef, undef, undef, undef} + {1, 1, 1, 1};
      arr[index..index+4] = %3
   }
}

I had to make up phony notation for vector instructions. Is there a more legitimate way to express this?

I am actually confused why there is a:

plus-8.ll

%index.next = add i64 %index, 8	
; I would have assumed + 4

[grosser]

Thanks for the test case. I have one additional comment. Otherwise, i agree with Siddharth's comments.

test/GPGPU/debug-metadata-leak.ll

24

Why does this test case have vector instructions in the first place? Even without vector instructions (just a simple copy loop), the same problem would exist, right?

[singam-sanjay]

Added pseudo code to test case.

[singam-sanjay]

test/GPGPU/debug-metadata-leak.ll

24

I had generated it using clang -O3 -mllvm -polly -mllvm -polly-sump-before...

[grosser]

Can you add -fno-vectorize and try again?

Best,
Tobias

[grosser]

Or even better use test/create_ll.sh?

Best,
Tobias

[singam-sanjay]

Simpler, Unoptimized and readable testcase.

[grosser]

My concerns are addressed. @Siddharth can you have a final look?

[bollu]

Other than nit, LGTM to me as well. Thanks!

test/GPGPU/debug-metadata-leak.ll

63

I believe the llvm.lifetime stuff is extra metadata that can be safely removed.

[grosser]

@sanjay: Any plans to commit this?

[singam-sanjay]

remove the llvm.lifetime* declarations

lib/CodeGen/BlockGenerators.cpp

if (!NewOperand) {
              "Store instructions are always needed!");
       NewInst->deleteValue();
       return;
     }
 
     NewInst->replaceUsesOfWith(OldOperand, NewOperand);
   }
 
+  // When copying the instruction onto the Module meant for the GPU,
+  // debug metadata attached to an instruction causes all related
+  // metadata to be pulled into the Module. This includes the DICompileUnit,
+  // which will not be listed in llvm.dbg.cu of the Module since the Module
+  // doesn't contain one. This fails the verification of the Module and the
+  // subsequent generation of the ASM string.
+  NewInst->setDebugLoc(llvm::DebugLoc());
+
   Builder.Insert(NewInst);
   BBMap[Inst] = NewInst;
 
   if (!NewInst->getType()->isVoidTy())
     NewInst->setName("p_" + Inst->getName());
 }
 
 Value *

test/GPGPU/debug-metadata-leak.ll

+; RUN: opt %loadPolly %s -polly-process-unprofitable -polly-codegen-ppcg -polly-acc-dump-kernel-ir \
+; RUN: | FileCheck --check-prefix=KERNEL-IR %s
+
+; REQUIRES: pollyacc
+
+; KERNEL-IR: define ptx_kernel void @FUNC_vec_add_1_SCOP_0_KERNEL_0(i8 addrspace(1)* %MemRef_arr, i32 %N) #0 {
+
+; The instruction marked <<<LeakyInst>>> is copied into the GPUModule,
+; with changes only to the parameters to access data on the device instead of
+; the host, i.e., MemRef_arr becomes polly.access.cast.MemRef_arr. Since the
+; instruction is annotated with a DILocation, copying the instruction also copies
+; the metadata into the GPUModule. This stops codegenerating the ptx_kernel by
+; failing the verification of the Module in GPUNodeBuilder::finalize, due to the
+; copied DICompileUnit not being listed in a llvm.dbg.cu which was neither copied
+; nor created.
+;
+; https://reviews.llvm.org/D35630 removes this debug metadata before the
+; instruction is copied to the GPUModule.
+; 
+; vec_add_1.c:
+;      void vec_add_1(int N, int arr[N]) {
+;        int i=0;
+;        for( i=0 ; i<N ; i++) arr[i] += 1;
+;      }
+;
+source_filename = "vec_add_1.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @vec_add_1(i32 %N, i32* %arr) !dbg !7 {
+entry:
+  call void @llvm.dbg.value(metadata i32 %N, i64 0, metadata !13, metadata !16), !dbg !17
+  call void @llvm.dbg.value(metadata i32* %arr, i64 0, metadata !14, metadata !16), !dbg !18
+  call void @llvm.dbg.value(metadata i32 0, i64 0, metadata !15, metadata !16), !dbg !19
+  %tmp = sext i32 %N to i64, !dbg !20
+  br label %for.cond, !dbg !20
+
+for.cond:                                         ; preds = %for.inc, %entry
+  %indvars.iv = phi i64 [ %indvars.iv.next, %for.inc ], [ 0, %entry ]
+  call void @llvm.dbg.value(metadata i32 undef, i64 0, metadata !15, metadata !16), !dbg !19
+  %cmp = icmp slt i64 %indvars.iv, %tmp, !dbg !22
+  br i1 %cmp, label %for.body, label %for.end, !dbg !24
+
+for.body:                                         ; preds = %for.cond
+  %arrayidx = getelementptr inbounds i32, i32* %arr, i64 %indvars.iv, !dbg !25
+  %tmp1 = load i32, i32* %arrayidx, align 4, !dbg !26, !tbaa !27
+  %add = add nsw i32 %tmp1, 1, !dbg !26    ;   <<<LeakyInst>>>  
+  store i32 %add, i32* %arrayidx, align 4, !dbg !26, !tbaa !27
+  br label %for.inc, !dbg !25
+
+for.inc:                                          ; preds = %for.body
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1, !dbg !31
+  call void @llvm.dbg.value(metadata !2, i64 0, metadata !15, metadata !16), !dbg !19
+  br label %for.cond, !dbg !32, !llvm.loop !33
+
+for.end:                                          ; preds = %for.cond
+  ret void, !dbg !35
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
+
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 5.0.0 (http://llvm.org/git/clang.git 23e042ffe07a923db2dbebf4d2a3692c5a454fee) (http://llvm.org/git/llvm.git 39c5686a1f54884f12120927b1753a750fdb5e02)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "vec_add_1.c", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 5.0.0 (http://llvm.org/git/clang.git 23e042ffe07a923db2dbebf4d2a3692c5a454fee) (http://llvm.org/git/llvm.git 39c5686a1f54884f12120927b1753a750fdb5e02)"}
+!7 = distinct !DISubprogram(name: "vec_add_1", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !12)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null, !10, !11}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !10, size: 64)
+!12 = !{!13, !14, !15}
+!13 = !DILocalVariable(name: "N", arg: 1, scope: !7, file: !1, line: 1, type: !10)
+!14 = !DILocalVariable(name: "arr", arg: 2, scope: !7, file: !1, line: 1, type: !11)
+!15 = !DILocalVariable(name: "i", scope: !7, file: !1, line: 2, type: !10)
+!16 = !DIExpression()
+!17 = !DILocation(line: 1, column: 20, scope: !7)
+!18 = !DILocation(line: 1, column: 27, scope: !7)
+!19 = !DILocation(line: 2, column: 7, scope: !7)
+!20 = !DILocation(line: 3, column: 8, scope: !21)
+!21 = distinct !DILexicalBlock(scope: !7, file: !1, line: 3, column: 3)
+!22 = !DILocation(line: 3, column: 15, scope: !23)
+!23 = distinct !DILexicalBlock(scope: !21, file: !1, line: 3, column: 3)
+!24 = !DILocation(line: 3, column: 3, scope: !21)
+!25 = !DILocation(line: 3, column: 25, scope: !23)
+!26 = !DILocation(line: 3, column: 32, scope: !23)
+!27 = !{!28, !28, i64 0}
+!28 = !{!"int", !29, i64 0}
+!29 = !{!"omnipotent char", !30, i64 0}
+!30 = !{!"Simple C/C++ TBAA"}
+!31 = !DILocation(line: 3, column: 21, scope: !23)
+!32 = !DILocation(line: 3, column: 3, scope: !23)
+!33 = distinct !{!33, !24, !34}
+!34 = !DILocation(line: 3, column: 35, scope: !21)
+!35 = !DILocation(line: 4, column: 1, scope: !7)