Page MenuHomePhabricator

Improve the API of DILocation::getMergedLocation()
AbandonedPublic

Authored by aprantl on Feb 10 2017, 10:03 AM.

Details

Summary

In a recent thread on LLVM-dev "Stripping Debug Locations on cross BB moves, part 2 (PR31891)" we discovered that the existing API for
getMergedLocation() is unsafe when used on CallInstructions. This patch moves the API from DILocation on Instruction so it automatically behaves correctly (i.e. return a line-0 debug loc) when used on a call instruction.

Once we agree on the exact API, I will split the patch up into an NFC commit that just moves the API and is a NOP on CallInstructions, and a second one that returns the line-0 location for CallInstructions.

Diff Detail

Event Timeline

aprantl created this revision.Feb 10 2017, 10:03 AM
dblaikie edited edge metadata.Feb 10 2017, 10:17 AM

Also we should have a function for the non-merging case (hoisting/sinking) so we can do similar things there (& presumably refactor the common infrastructure between these two operations).

lib/IR/Instruction.cpp
664–668

I'm not sure we can get the scope right, though - since we're potentially moving this location across scopes. It's going to be jumpy/create a difficult range in the scope no matter what, really...

Any ideas? Take the scope from a nearby location at the destination, if possible? (doesn't really reflect reality, but nothing will - this would at least mean a chance of not punching holes in/making islands in scopes that would necessitate a DW_AT_ranges/more verbose description/etc)

rob.lougher edited reviewers, added: rob.lougher; removed: dblaikie.Feb 10 2017, 10:20 AM
twoh added a subscriber: twoh.Feb 14 2017, 9:33 AM
twoh added inline comments.Feb 14 2017, 10:44 AM
lib/IR/Instruction.cpp
664–668

It would be hard to get the precise scope anyway, but I think it might be worth to consider the inlining context. Please take a look at the following test case:

define i32 @bar() !dbg !6 {
entry:
  %call = tail call i32 @foo(), !dbg !8
  ret i32 %call, !dbg !9
}

define i32 @test(i32 %a, i32 %b) !dbg !10 {
entry:
  %tobool = icmp ne i32 %a, 0, !dbg !11
  br i1 %tobool, label %if.then, label %if.else, !dbg !11

if.then:                                          ; preds = %entry
  %call.i = call i32 @foo(), !dbg !12
  %sub = sub nsw i32 %b, %call.i, !dbg !14
  br label %if.end, !dbg !15

if.else:                                          ; preds = %entry
  %call1 = call i32 @foo(), !dbg !16
  %sub2 = sub nsw i32 %b, %call1, !dbg !17
  br label %if.end

if.end:                                           ; preds = %if.else, %if.then
  %b.addr.0 = phi i32 [ %sub, %if.then ], [ %sub2, %if.else ]
  ret i32 %b.addr.0, !dbg !18
}

where

!6 = distinct !DISubprogram(name: "bar", ...)
!10 = distinct !DISubprogram(name: "test", ...)
!12 = !DILocation(line: 4, column: 10, scope: !6, inlinedAt: !13)
!16 = !DILocation(line: 11, column: 10, scope: !10)

Here the call instruction in %if.then block is actually inlined from @bar, and if we pickup the scope from that instruction, the final code would be look something like below:

define i32 @test(i32 %a, i32 %b) !dbg !10 {
entry:
  %call.i = call i32 @foo(), !dbg !11
  %sub = sub nsw i32 %b, %call.i, !dbg !13
  ret i32 %sub, !dbg !14
}

!6 = distinct !DISubprogram(name: "bar", ...)
!11 = !DILocation(line: 0, column: 0, scope: !6, inlinedAt: !12)

As the call instructions are merged and hoisted inside test not bar, it would be awkward if the merged call instruction has a scope of bar, though the debug location is still just (line: 0, column: 0).

I missed the discussion in llvm-dev and implemented a patch for the same issue considering inline scope (D29927). Maybe worth take a look.

Ping! Sorry, I've been distracted. How do we want to proceed here?

I think the first action item we should get out of the way is:

  • Do we think that moving mergeDebugLocWith into Instruction is a a good idea?

Once we decide on that issue, let's talk about how to best merge D29833 and D29927.

dblaikie edited edge metadata.Feb 21 2017, 10:28 AM

I don't have strong feelings about where this function lives. Still unsure about preserving the scope from either of the merged locations, though.

twoh added a comment.Feb 21 2017, 11:26 AM

Personally I prefer the existing API, because it doesn't allow the transient state before merging. For example, in lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp in this patch, DebugLoc of NewSI is not actually valid after line 1430. Existing API doesn't have this problem.

I don't have strong opinion about scope, because anyway we won't be able to have precise scope. Taking DISubprogram of the destination instruction might be a conservative option.

probinson edited edge metadata.Feb 21 2017, 2:56 PM
In D29833#682595, @twoh wrote:

I don't have strong opinion about scope, because anyway we won't be able to have precise scope. Taking DISubprogram of the destination instruction might be a conservative option.

Mmmm... Does scope imply source file? LTO can inline across CUs, then merging an instruction from CU#1 into a scope from CU#2 would make the line number meaningless.

twoh added a comment.Feb 21 2017, 3:38 PM
In D29833#682595, @twoh wrote:

I don't have strong opinion about scope, because anyway we won't be able to have precise scope. Taking DISubprogram of the destination instruction might be a conservative option.

Mmmm... Does scope imply source file? LTO can inline across CUs, then merging an instruction from CU#1 into a scope from CU#2 would make the line number meaningless.

My understanding is that if two instructions come from different source file, canDiscriminate will return true because filenames are different and the merged metadata will have a line 0 location.

[apologies for iterating so slowly at the moment]

In D29833#682595, @twoh wrote:

Personally I prefer the existing API, because it doesn't allow the transient state before merging. For example, in lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp in this patch, DebugLoc of NewSI is not actually valid after line 1430. Existing API doesn't have this problem.

If that is the only concern, would an additional convenience function solve (= sufficiently hide) the problem?

Instruction::setMergedDebugLoc(DebugLoc A, DebugLoc B) {
  setDebugLoc(A);
  mergeDebugLocWith(B);
}

The primary benefit of having the API on Instruction is that we can make it impossible to accidentally create a locationless call instruction in a function with debuginfo (which would crash/assert the backend when creating inline scopes).

twoh added a comment.Feb 23 2017, 9:13 AM

@aprantl I like the idea of having a convenience function. I'm afraid I still don't understand @dblaikie on getMergedLocation is better than the proposed convenience function in preventing buggy profile.

aprantl abandoned this revision.Feb 23 2017, 9:36 AM

I'm abandoning this revision in favor of extending getMergedLocation as discussed in D29927.

pmatos added a subscriber: pmatos.Oct 19 2017, 2:00 AM