Do not model debuginfo intrinsics in MemorySSA.
Regularly these are non-memory modifying instructions. With -disable-basicaa, they were being modelled as Defs.
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 37971 Build 37970: arc lint + arc unit
Event Timeline
lib/Analysis/MemorySSA.cpp | ||
---|---|---|
1744 | If you are not going to use a switch here, dyn_cast<DbgInfoIntrinsic>() is shorter. |
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 | 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) | |
1738 | 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 |
lib/Analysis/MemorySSA.cpp | ||
---|---|---|
292 | This should be llvm_unreachable(). |
test/Analysis/MemorySSA/debugvalue2.ll | ||
---|---|---|
3 | Is there nothing other than checking for no assertion that we could check for in the output? |
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 | please remove the assert; llvm_unreachable alone should suffice. |
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)