This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking][SROA] Handle createFragmentExpression failure
ClosedPublic

Authored by Orlando on Apr 3 2023, 3:16 AM.

Details

Summary

createFragmentExpression will fail if it determines that the expression cannot be split over fragments. Handle this case in SROA. Similarly to D147312 this should be a rare occurrence as the dbg.assign will usually reference the Value being stored without modifying it with a DIExpression.

Diff Detail

Event Timeline

Orlando created this revision.Apr 3 2023, 3:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2023, 3:16 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Orlando requested review of this revision.Apr 3 2023, 3:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2023, 3:16 AM
jmorse accepted this revision.Apr 3 2023, 3:40 AM

LGTM, in that it's avoiding an assertion through graceful degradation of the debug-info. Out of curiosity, for what reason can't a fragment expression be created in the test?

This revision is now accepted and ready to land.Apr 3 2023, 3:40 AM

Thanks. It's the DW_OP_plus_uconst in the test causing createFragmentExpression to fail -- see createFragmentExpression in DebugInfoMetadata.cpp, which avoids splitting arithmetic expressions.

StephenTozer added inline comments.Apr 3 2023, 4:02 AM
llvm/test/DebugInfo/Generic/assignment-tracking/sroa/fail-fragment.ll
11

I'm curious - why is this value not poison? I'm assuming it's the upper fragment for the dbg.assign that couldn't be split, so I'd have thought it would be a kill location as well.

14–15

Tiniest nit.

I'm curious - why is this value not poison? I'm assuming it's the upper fragment for the dbg.assign that couldn't be split, so I'd have thought it would be a kill location as well.

Good question. It turns out it is poison in the call to migrateDebugInfo when the store is split, but then in another call to migrateDebugInfo later the poison value gets replaced. Nice spot.

The easy fix is to preserve poison-ness of the dbg.assign. However, a dbg.assign linked to a memcpy will have a poison value component. If SROA replaces that memcpy with stores the behaviour at the moment is to replace the poison with the stored-value.

The underlying issue here for both the original issue I was fixing and this poison situation is that the dbg.assign value isn't represented in the same way as the stored value. I think the fix for the original issue makes sense, the "easy fix" mention above also makes sense, and it's the memcpy-linked dbg.assign value representation that doesn't quite fit.

Not quite sure how to solve this just yet - ideas are welcome!

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/fail-fragment.ll
11

(replying with a main-line comment)

This revision was landed with ongoing or failed builds.Apr 5 2023, 3:20 AM
This revision was automatically updated to reflect the committed changes.
Orlando added inline comments.Apr 5 2023, 3:24 AM
llvm/test/DebugInfo/Generic/assignment-tracking/sroa/fail-fragment.ll
11

I've added a FIXME to the test. I've landed this as-is for now to fix the assertion and live with this edge-case, I'll come back to this.

14–15

I turned the other CHECKS into the shorter-hand version