This is an archive of the discontinued LLVM Phabricator instance.

Remove Debug metadata from copied instruction to prevent Module verification failure
ClosedPublic

Authored by singam-sanjay on Jul 19 2017, 9:50 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

singam-sanjay created this revision.Jul 19 2017, 9:50 AM
grosser edited edge metadata.Jul 19 2017, 9:54 AM

Amazing. Can you add a test case?

grosser requested changes to this revision.Jul 19 2017, 9:38 PM

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

This revision now requires changes to proceed.Jul 19 2017, 9:38 PM
singam-sanjay edited edge metadata.

Added test case.

bollu added inline comments.Jul 21 2017, 12:58 AM
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 requested changes to this revision.Jul 21 2017, 10:21 AM

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?

This revision now requires changes to proceed.Jul 21 2017, 10:21 AM
singam-sanjay edited edge metadata.

Added pseudo code to test case.

singam-sanjay marked 4 inline comments as done.Jul 21 2017, 10:48 AM
singam-sanjay added inline comments.
test/GPGPU/debug-metadata-leak.ll
24

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

Can you add -fno-vectorize and try again?

Best,
Tobias

Or even better use test/create_ll.sh?

Best,
Tobias

grosser requested changes to this revision.Jul 21 2017, 10:51 AM
This revision now requires changes to proceed.Jul 21 2017, 10:51 AM
singam-sanjay edited edge metadata.
singam-sanjay marked an inline comment as done.

Simpler, Unoptimized and readable testcase.

grosser accepted this revision.Jul 22 2017, 2:01 PM

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

This revision is now accepted and ready to land.Jul 22 2017, 2:01 PM
bollu accepted this revision.Jul 25 2017, 1:27 AM

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.

@sanjay: Any plans to commit this?

remove the llvm.lifetime* declarations

This revision was automatically updated to reflect the committed changes.