This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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. Something similar happens if the starting access is a load with a MemoryDef.

Change the implementation to not set Q.Inst in the first place if we want to perform a MemoryLocation-based query, to make sure it can't be turned into an Instruction-based query along the way...

Additionally, remove the special handling that lifetime.start intrinsics currently get. They simply report NoAlias for clobbers between lifetime.start and other calls, but that's obviously not correct if the other call is something like a memset or memcpy. The default behavior we get from getModRefInfo() will already do the right thing here.

Diff Detail

Event Timeline

nikic created this revision.Oct 3 2020, 8:35 AM
nikic requested review of this revision.Oct 3 2020, 8:35 AM
nikic planned changes to this revision.Oct 3 2020, 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.Oct 3 2020, 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.Oct 5 2020, 5:13 PM
llvm/lib/Analysis/MemorySSA.cpp
2359

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

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

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.Oct 7 2020, 2:58 PM

Add a dedicated test with queries starting from a memset.

nikic added inline comments.Oct 7 2020, 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.Oct 9 2020, 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.

nikic edited the summary of this revision. (Show Details)Oct 30 2020, 2:01 AM
asbirlea accepted this revision.Nov 3 2020, 6:33 PM

Thank you for detailed tests, this looks good!

This revision is now accepted and ready to land.Nov 3 2020, 6:33 PM