Page MenuHomePhabricator

[Instruction] Add updateLocationAfterHoist helper
ClosedPublic

Authored by vsk on Aug 10 2020, 10:46 AM.

Details

Summary

Introduce a helper which can be used to update the debug location of an
Instruction after the instruction is hoisted. This can be used to safely
drop a source location as recommended by the docs.

For more context, see the discussion in https://reviews.llvm.org/D60913.

Diff Detail

Event Timeline

vsk created this revision.Aug 10 2020, 10:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2020, 10:46 AM
vsk requested review of this revision.Aug 10 2020, 10:46 AM
vsk updated this revision to Diff 284442.

Add doxygen comment.

aprantl accepted this revision.Aug 10 2020, 1:12 PM

Thank you!

This revision is now accepted and ready to land.Aug 10 2020, 1:12 PM
This revision was landed with ongoing or failed builds.Aug 11 2020, 2:05 PM
This revision was automatically updated to reflect the committed changes.
vsk reopened this revision.Aug 11 2020, 2:59 PM
vsk added inline comments.
llvm/lib/IR/DebugInfo.cpp
714

This causes some bots to fail with "attachment points at wrong subprogram for function" verifier errors. The verifier check tests that a location's inlinedAt scope describes the parent function it's in. I believe the correct way to handle calls which have a location is to set the parent function's scope on the call (if the parent function has a scope), and to fall back to using DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()) if not.

This revision is now accepted and ready to land.Aug 11 2020, 2:59 PM
vsk edited the summary of this revision. (Show Details)Aug 11 2020, 3:38 PM
vsk updated this revision to Diff 284921.Aug 11 2020, 3:39 PM

Preserve the inlinedAt location unless the parent function has a scope.

aprantl added inline comments.Aug 12 2020, 4:56 PM
llvm/lib/IR/DebugInfo.cpp
717

Should we pick up a scope from the instruction before it to avoid having a hole in that scope's range?

vsk updated this revision to Diff 290577.Sep 8 2020, 1:51 PM

Add a dropLocation API, and use this to implement updateLocationAfterHoist. 'dropLocation' is a more general name, and should be applicable in more contexts.

llvm/lib/IR/DebugInfo.cpp
717

I'd argue that we shouldn't. In this case, I think we want to use a scope that conveys 'you could be anywhere in this function'. The scope from a nearby instruction could be more specific than that.

dblaikie added inline comments.Sep 9 2020, 2:12 PM
llvm/lib/IR/DebugInfo.cpp
717

Yeah, this one's unfortunate - when merging, we can find the nearest common scope and that's at least somewhat meaningful. But when hoisting that's not so clearly correct. :/

723–724

This alternative seems a bit problematic, as it may lead to differences in location depending on the order of inlining, which might be problematic/volatile/unpredictable for developers? but maybe there's no good answer. The other option only seems to be "drop the location entirely" (I think that's correct in the case where it isn't in a function with debug info? IF it gets inlined into such a function, it'll get a location at that point?)

vsk updated this revision to Diff 293896.Wed, Sep 23, 4:48 PM
  • To drop the location of a call within a function with no subprogram, set an empty DebugLoc instead of trying to preserve scope/inlinedAt info. Update a unit test accordingly.
vsk added inline comments.Wed, Sep 23, 4:52 PM
llvm/lib/IR/DebugInfo.cpp
723–724

Thanks for the suggestion. I wasn't aware that the inliner provided a location when inlining a call without a location from a function without a subprogram. But I suppose it must, because the inlined call may reference a function that /does/ have a subprogram attached.

Dropping the location seems less arbitrary than preserving the old scope/inlinedAt info.

dblaikie accepted this revision.Wed, Sep 23, 6:36 PM

Seems good to me - minor tidy up might be possible in the unit test.

llvm/unittests/IR/InstructionsTest.cpp
1350

Maybe these should be EXPECT_EQ(I1->getDebugLoc(), DebugLoc());? Would that work? Would hopefully give better failure messages by printing the values when the expect fails.

vsk added inline comments.Thu, Sep 24, 2:46 PM
llvm/unittests/IR/InstructionsTest.cpp
1350

That does seem cleaner, thanks. I'll fix this up before committing.