[PATCH] D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass.

Tue Jul 16 14:29:15 PDT 2019

avl added a comment.

@jmorse Thank you for this case and explanations. I will evaluate it more and adapt fix accordingly.

> However, IMHO converting dbg.values to dbg.declares is going to prove problematic. 
>  There's no (AFAIK) good way to confirm that a dbg.value was originally _supposed_ to be a dbg.declare, 
>  and so dbg.values that accidentally refer to an alloca might be converted. 
>  There are all kinds of optimisations and salvaging that could make dbg.values point back at an alloca.

Probably, it is OK if dbg.value was not originally _supposed_ to be a dbg.declare but was converted in this place. 
if SROA is able to do optimization then such dbg.declare would be converted into dbg.value later in  PromoteMemToReg.
i.e. If after all optimizations  dbg.value relates to valid slice of alloca - then it is OK to match it with that slice inserting dbg.declare, 
which would be replaced with dbg.value.

This patch uses incorrect dbg.value for your example and finally debug information for "ptr" put in the result.
That("ptr") actually should be removed.  If it would take another dbg.value as the basis then the result would be correct.

Let`s check IR :

before first SROA

    %retval = alloca i32, align 4
  %result = alloca %struct.S1, align 8
  %ptr = alloca %struct.S1*, align 8
  %cleanup.dest.slot = alloca i32, align 4
  %0 = bitcast %struct.S1* %result to i8*, !dbg !25
  call void @llvm.lifetime.start.p0i8(i64 16, i8* %0) #5, !dbg !25
  call void @llvm.dbg.declare(metadata %struct.S1* %result, metadata !12, metadata !DIExpression()), !dbg !26
  %call = call { i32, i64 } @_Z3foov(), !dbg !27
  %1 = bitcast %struct.S1* %result to { i32, i64 }*, !dbg !27
  %2 = getelementptr inbounds { i32, i64 }, { i32, i64 }* %1, i32 0, i32 0, !dbg !27
  %3 = extractvalue { i32, i64 } %call, 0, !dbg !27
  store i32 %3, i32* %2, align 8, !dbg !27
  %4 = getelementptr inbounds { i32, i64 }, { i32, i64 }* %1, i32 0, i32 1, !dbg !27
  %5 = extractvalue { i32, i64 } %call, 1, !dbg !27
  store i64 %5, i64* %4, align 8, !dbg !27
  %6 = bitcast %struct.S1** %ptr to i8*, !dbg !28
  call void @llvm.lifetime.start.p0i8(i64 8, i8* %6) #5, !dbg !28
  call void @llvm.dbg.declare(metadata %struct.S1** %ptr, metadata !23, metadata !DIExpression()), !dbg !29
  store %struct.S1* %result, %struct.S1** %ptr, align 8, !dbg !29, !tbaa !30
  %call1 = call zeroext i1 @_ZN2S16IsNullEv(%struct.S1* %result), !dbg !34
  br i1 %call1, label %if.then, label %if.end, !dbg !3

after first SROA

  %result = alloca %struct.S1, align 8
  %0 = bitcast %struct.S1* %result to i8*, !dbg !25
  call void @llvm.lifetime.start.p0i8(i64 16, i8* %0) #5, !dbg !25
  call void @llvm.dbg.declare(metadata %struct.S1* %result, metadata !12, metadata !DIExpression()), !dbg !26
  %call = call { i32, i64 } @_Z3foov(), !dbg !27
  %1 = bitcast %struct.S1* %result to { i32, i64 }*, !dbg !27
  %2 = getelementptr inbounds { i32, i64 }, { i32, i64 }* %1, i32 0, i32 0, !dbg !27
  %3 = extractvalue { i32, i64 } %call, 0, !dbg !27
  store i32 %3, i32* %2, align 8, !dbg !27
  %4 = getelementptr inbounds { i32, i64 }, { i32, i64 }* %1, i32 0, i32 1, !dbg !27
  %5 = extractvalue { i32, i64 } %call, 1, !dbg !27
  store i64 %5, i64* %4, align 8, !dbg !27
  call void @llvm.dbg.value(metadata %struct.S1* %result, metadata !23, metadata !DIExpression()), !dbg !28
  %call1 = call zeroext i1 @_ZN2S16IsNullEv(%struct.S1* %result), !dbg !29
  br i1 %call1, label %if.then, label %if.end, !dbg !31

before second SROA

  %result = alloca %struct.S1, align 8
  %0 = bitcast %struct.S1* %result to i8*, !dbg !25
  call void @llvm.lifetime.start.p0i8(i64 16, i8* nonnull %0) #4, !dbg !25
  %call = call { i32, i64 } @_Z3foov(), !dbg !26
  %1 = getelementptr inbounds %struct.S1, %struct.S1* %result, i64 0, i32 0, !dbg !26
  %2 = extractvalue { i32, i64 } %call, 0, !dbg !26
  store i32 %2, i32* %1, align 8, !dbg !26
  %3 = getelementptr inbounds %struct.S1, %struct.S1* %result, i64 0, i32 1, !dbg !26
  %4 = extractvalue { i32, i64 } %call, 1, !dbg !26
  store i64 %4, i64* %3, align 8, !dbg !26
  call void @llvm.dbg.value(metadata %struct.S1* %result, metadata !23, metadata !DIExpression()), !dbg !27
  call void @llvm.dbg.value(metadata %struct.S1* %result, metadata !12, metadata !DIExpression(DW_OP_deref)), !dbg !27
  call void @llvm.dbg.value(metadata %struct.S1* %result, metadata !28, metadata !DIExpression()), !dbg !31
  %cmp.i = icmp eq i32 %2, 0, !dbg !34
  br i1 %cmp.i, label %if.then, label %if.end, !dbg !35

after second SROA

  call void @llvm.dbg.declare(metadata !2, metadata !23, metadata !DIExpression(DW_OP_LLVM_fragment, 32, 32)), !dbg !25
  %call = call { i32, i64 } @_Z3foov(), !dbg !26
  %0 = extractvalue { i32, i64 } %call, 0, !dbg !26
  call void @llvm.dbg.value(metadata i32 %0, metadata !23, metadata !DIExpression(DW_OP_LLVM_fragment, 0, 32)), !dbg !25
  %1 = extractvalue { i32, i64 } %call, 1, !dbg !26
  call void @llvm.dbg.value(metadata i64 %1, metadata !12, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !25
  %cmp.i = icmp eq i32 %0, 0, !dbg !27
  br i1 %cmp.i, label %if.then, label %if.end, !dbg !33

before second SROA it has three dbg.values pointing to the same alloca :

"ptr:bar"  call void @llvm.dbg.value(metadata %struct.S1* %result, metadata !23, metadata !DIExpression()), !dbg !27
"result:bar":   call void @llvm.dbg.value(metadata %struct.S1* %result, metadata !12, metadata !DIExpression(DW_OP_deref)), !dbg !27
"this:foo"  call void @llvm.dbg.value(metadata %struct.S1* %result, metadata !28, metadata !DIExpression()), !dbg !31

"ptr:bar" and "this:foo"  should be removed.
"result:bar" should be used as the alloca instruction for fragments. thus there should be inserted corresponding dbg.declare for fragments which would be promoted into dbg.values in PromoteMemToReg.

function which would select proper dbg.value should probably be based on variable, expression and fragment.
Probably it could refuse those dbg.values which do not dereference the alloca address and do not have an aggregate type as you suggested.

> One potential workaround might be to examine the DIExpression to see whether or not it actually dereferences the alloca address and has an aggregate type. However that means digging through the DIExpression operands, which I believe can become complicated if a lot of optimization has happened. There might also be non-C/C++ frontends that expect to be able to temporarily bind variable names to memory, then move them later. Converting their dbg.values to dbg.declares would change the duration that the variable referred to a particular piece of memory.

I would give it a try. i.e. I will write a function which would examine DIExpression to decide whether dbg.value should be deleted or not.
If I understood correctly - converting dbg.gvalues into dbg.declare should not create a problem since all these dbg.declare should be changed in dbg.values in PromoteMemToReg.

> Instead, maybe we could just not convert dbg.declare for structs to dbg.values during instcombine in the first place? While digging into this I noticed that the comment at [0] says only scalars are supposed to be dbg.declare-converted during instcombine, but the code allows structs to slip through. I don't know the history of LowerDbgDeclare though, do other debug-info people know why structs get converted there?

I think it would be good to not to limit conversions. There could probably be another optimizations(not only instcombine) which would produce dbg.values. SROA itself produces dbg.values.
Thus it should be able to work with them if apllied twice.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64595/new/

https://reviews.llvm.org/D64595