This is an archive of the discontinued LLVM Phabricator instance.

[MemorySSA] Do not create memoryaccesses for debug info intrinsics.
ClosedPublic

Authored by asbirlea on Sep 6 2019, 4:17 PM.

Details

Summary

Do not model debuginfo intrinsics in MemorySSA.
Regularly these are non-memory modifying instructions. With -disable-basicaa, they were being modelled as Defs.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Sep 6 2019, 4:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2019, 4:17 PM
aprantl added inline comments.Sep 6 2019, 4:23 PM
lib/Analysis/MemorySSA.cpp
1740 ↗(On Diff #219197)

If you are not going to use a switch here, dyn_cast<DbgInfoIntrinsic>() is shorter.

asbirlea updated this revision to Diff 219201.Sep 6 2019, 4:30 PM
asbirlea marked an inline comment as done.

Address comment.

Any chance of test coverage? (not sure how the rest of this analysis is tested (or how we test analyses in general))

Thanks for this!

+1 for a simple -disable-basicaa test please.

lib/Analysis/MemorySSA.cpp
287 ↗(On Diff #219201)

this should be dynamically dead code, no? II is sourced from MD, so there has to be an existing Def for these. if we're special-casing elsewhere to ensure these Defs are never created, ...

(if you'd rather make these new cases lead to assert(false && "debuginfo shouldn't have associated defs!"); or similar, i'm equally content with that)

1734 ↗(On Diff #219201)

nit: please include a note saying something like debuginfo intrinsics may be considered clobbers when we have a nonstandard AA pipeline, so we special-case them

asbirlea updated this revision to Diff 219346.Sep 9 2019, 7:24 AM
asbirlea marked 2 inline comments as done.

Add testcase.
Address comments.

aprantl accepted this revision.Sep 9 2019, 2:19 PM
aprantl added inline comments.
lib/Analysis/MemorySSA.cpp
292 ↗(On Diff #219346)

This should be llvm_unreachable().

This revision is now accepted and ready to land.Sep 9 2019, 2:19 PM
aprantl added inline comments.Sep 9 2019, 2:20 PM
test/Analysis/MemorySSA/debugvalue2.ll
2 ↗(On Diff #219346)

Is there nothing other than checking for no assertion that we could check for in the output?

aprantl requested changes to this revision.Sep 9 2019, 2:21 PM
This revision now requires changes to proceed.Sep 9 2019, 2:21 PM
asbirlea updated this revision to Diff 219574.Sep 10 2019, 11:15 AM
asbirlea marked 2 inline comments as done.

Address comments.
Updated test to check no MemoryDef exists. Added comment to leave a trail on how to reproduce the failure if debuginfo is modelled in MSSA.

thanks!

lib/Analysis/MemorySSA.cpp
293 ↗(On Diff #219574)

please remove the assert; llvm_unreachable alone should suffice.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 10 2019, 3:34 PM
This revision was automatically updated to reflect the committed changes.