Page MenuHomePhabricator

[MemorySSA] Use provided memory location even if instruction is call
Needs ReviewPublic

Authored by nikic on Sat, Oct 3, 8:35 AM.

Details

Reviewers
asbirlea
fhahn
Summary

If getClobberingMemoryAccess() is called with an explicit MemoryLocation, but the starting access happens to be a call, the provided location is currently ignored, and alias analysis queries will be performed against the call instruction instead. Change the implementation to always use the location if provided, and only use the call for IsCall queries.

Diff Detail

Event Timeline

nikic created this revision.Sat, Oct 3, 8:35 AM
nikic requested review of this revision.Sat, Oct 3, 8:35 AM
nikic planned changes to this revision.Sat, Oct 3, 10:30 AM

Okay, I'm pretty sure this is not fixing the right issue. If I'm understanding correctly, the real problem is that the provided memory location just gets ignored entirely if the UseInst happens to be a call. I think whether AA is queried with the location or with the call should be determined by Query.IsCall, not whether UseInst is a call.

nikic updated this revision to Diff 295992.Sat, Oct 3, 11:02 AM
nikic retitled this revision from [MemorySSA] Consistently handle lifetime.start clobbers to [MemorySSA] Use provided memory location even if instruction is call.
nikic edited the summary of this revision. (Show Details)

Fix the underlying problem. Ran into this with lifetime intrinsics, but it's really something that affects all calls.

asbirlea added inline comments.Mon, Oct 5, 5:13 PM
llvm/lib/Analysis/MemorySSA.cpp
2354

Is it possible this is the place where the inconsistency comes from, or did I misunderstand?

nikic added inline comments.Tue, Oct 6, 12:37 AM
llvm/lib/Analysis/MemorySSA.cpp
2354

Depends on what you mean by "comes from" :) This is indeed the place where we create a Query with IsCall=false and Inst being (potentially) a CallBase. However, I believe this is correct, as this query should only care about the provided Loc, not the Inst it happens to start at. Maybe it would be possible to set Inst to nullptr altogether...

nikic updated this revision to Diff 296803.Wed, Oct 7, 2:58 PM

Add a dedicated test with queries starting from a memset.

nikic added inline comments.Wed, Oct 7, 3:06 PM
llvm/lib/Analysis/MemorySSA.cpp
301

Even after this fix, there will still be an issue with the handling of loads, due to this code. Say we start from a store and perform a clobber query on getDefiningAccess(), which happens to return a volatile load. Then we will end up trying to find clobbers for the volatile load, once again ignoring the provided location completely.

nikic updated this revision to Diff 297286.Fri, Oct 9, 11:17 AM

Set Inst to nullptr to prevent any abuse, thus handling both the call and the load case. Also drop the special lifetime.start handling so that clobbers between calls and lifetime.start are correctly handled.