Page MenuHomePhabricator

[MemorySSA] Update MSSA for non-conventional AA.
ClosedPublic

Authored by asbirlea on Sep 13 2019, 10:26 AM.

Details

Summary

Regularly when moving an instruction that may not read or write memory,
the instruction is not modelled in MSSA, so not action is necessary.
For a non-conventional AA pipeline, MSSA must be updated with the move.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Sep 13 2019, 10:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 10:26 AM

Why does an instruction which doesn't read or write memory have an associated MemorySSA memory access? Do we assume everything accesses memory if basicaa is disabled? Would it make sense to fix that, instead of adding checks which always fail normally?

Why does an instruction which doesn't read or write memory have an associated MemorySSA memory access? Do we assume everything accesses memory if basicaa is disabled?

Except for a few cases, we rely on getModRefInfo to add MemoryAccesses to Instructions, so, yes, without basicaa, we're creating accesses where we shouldn't.

Would it make sense to fix that, instead of adding checks which always fail normally?

Yes, that would make a lot of sense! I was thinking of adding this as as alternative in the patch description so I'm very happy you suggested it.

What I'm conflicted is about is how much "pruning" of things to add when MSSA is creating an access. I don't want to duplicate basicaa inside MemorySSA just because we may run without basicaa, but I also don't want to end up adding checks everywhere for the -disable-basicaa case.
As a first step I can take the conditions !Inst->mayReadFromMemory() && !Inst->mayWriteToMemory() and never create accesses for those, but I'm thinking I may still be missing cases where we're disabling basicaa and not making updates.

I guess in theory it's possible, for example, for a load to be ModRefInfo::NoModRef because it loads from a readonly global, and to write a pass that depends on the assumption that we won't create a MemoryUse for that load. (I think we actually create a MemoryUse for that at the moment, but it could change.)

In practice, excluding instructions that don't access memory is probably good enough to cover all the relevant cases for passes that use MemorySSA.

asbirlea updated this revision to Diff 220195.Fri, Sep 13, 5:28 PM

Make the update in MemorySSA.

thanks for this!

i like the new approach much more :)

lib/Analysis/MemorySSA.cpp
1742 ↗(On Diff #220195)

does obviate the need for the recently-added isa<DbgInfoIntrinsic>()?

if so, please add a comment noting that nonstandard AA pipelines might leave us with silly modref results for I, so this check is necessary for correctness.

test/Analysis/MemorySSA/loop-rotate-disablebasicaa.ll
5 ↗(On Diff #220195)

world's smallest nit: s/define void/define void/

10 ↗(On Diff #220195)

should we be CHECK-NOT'ing a MemoryDef for this? seems more direct than expecting a verifier error.

asbirlea updated this revision to Diff 220381.Mon, Sep 16, 1:13 PM
asbirlea marked 3 inline comments as done.

Address comments.

Thanks!

lib/Analysis/MemorySSA.cpp
1742 ↗(On Diff #220195)

(sorry: please also remove the isa<DbgInfoIntrinsic>() above, since it's now redundant :) )

This revision is now accepted and ready to land.Mon, Sep 16, 8:49 PM
This revision was automatically updated to reflect the committed changes.
asbirlea marked an inline comment as done.