This is an archive of the discontinued LLVM Phabricator instance.

[Sink] Don't move calls to readonly functions across stores
ClosedPublic

Authored by nhaehnle on Feb 15 2016, 2:46 PM.

Details

Summary

This is conservative because it doesn't consider the argmemonly attribute.
Perhaps somebody who knows this part of the code better can improve this
in a separate patch. For now, it fixes a real miscompilation I encountered
in the AMDGPU backend.

Diff Detail

Event Timeline

nhaehnle updated this revision to Diff 48017.Feb 15 2016, 2:46 PM
nhaehnle retitled this revision from to [Sink] Don't move calls to readonly functions across stores.
nhaehnle updated this object.
nhaehnle added a subscriber: llvm-commits.

Doesn't AA have an existing API that gives you exactly the info you want
here (Does this call ref the memory touched the things in the store list)

Doesn't AA have an existing API that gives you exactly the info you want
here (Does this call ref the memory touched the things in the store list)

I see no such interface in AAResults at least. All queries test $something against a MemoryLocation, but the Stores are instructions because they could also be call sites. There would have to be an interface to test an instruction against an instruction, or two call sites against each other.

hfinkel edited edge metadata.Mar 10 2016, 12:21 PM

Doesn't AA have an existing API that gives you exactly the info you want
here (Does this call ref the memory touched the things in the store list)

I see no such interface in AAResults at least. All queries test $something against a MemoryLocation, but the Stores are instructions because they could also be call sites. There would have to be an interface to test an instruction against an instruction, or two call sites against each other.

we have:

/// getModRefInfo (for call sites) - Return information about whether
/// a particular call site modifies or reads the specified memory location.
ModRefInfo getModRefInfo(ImmutableCallSite CS, const MemoryLocation &Loc);

/// getModRefInfo (for calls) - Return information about whether
/// a particular call modifies or reads the specified memory location.
ModRefInfo getModRefInfo(const CallInst *C, const MemoryLocation &Loc) {
  return getModRefInfo(ImmutableCallSite(C), Loc);
}

and many others. Also, the the generic wrapper:

ModRefInfo getModRefInfo(const Instruction *I, const MemoryLocation &Loc) {

all in include/llvm/Analysis/AliasAnalysis.h.

Right, and none of these test an instruction against another instruction, or (which would be sufficient) a callsite against a callsite.

A different way to think about it is that there is no existing interface to get a list of MemoryLocations from a callsite.

Right, and none of these test an instruction against another instruction, or (which would be sufficient) a callsite against a callsite.

Sure there is:

ModRefInfo getModRefInfo(ImmutableCallSite CS1, ImmutableCallSite CS2);

There is also this:

ModRefInfo getModRefInfo(Instruction *I, ImmutableCallSite Call);

there is also a MemoryLocation::get(const Instruction *Inst) - which handles all of the different instruction types except calls (there are more-refined getters for calls, chiefly getForArgument).

A different way to think about it is that there is no existing interface to get a list of MemoryLocations from a callsite.

nhaehnle updated this revision to Diff 55294.Apr 27 2016, 12:38 PM
nhaehnle edited edge metadata.

Update and change to deal with argmemonly.

This required a subtle change to getModRefInfo(Instruction, ImmutableCallSite)
to ensure that the semantics are equal to that of getModRefInfo(CS1, CS2) when
the Instruction is a call-site.

I would consider the old behavior to be a bug, which seems to not have surfaced
because it seems the only user of that particular getModRefInfo overload is in
MemorySSA, and that user only checks the return value for != MR_NoModRef.

While the relation is non-commutative, changing the order of the call-sites
may change the result from MR_Ref to MR_Mod and vice versa, but never to
MR_NoModRef.

Technically I guess the change in getModRefInfo isn't needed, but it seems like the Right Thing to me.

dberlin accepted this revision.May 31 2016, 8:03 AM
dberlin added a reviewer: dberlin.

Please add a test for the getmodref info change and commit it separately.

lib/Analysis/AliasAnalysis.cpp
112

Please commit this separately.

This revision is now accepted and ready to land.May 31 2016, 8:03 AM
This revision was automatically updated to reflect the committed changes.

Please see https://llvm.org/bugs/show_bug.cgi?id=28570 for a bug reported bisected back to this revision.