This is an archive of the discontinued LLVM Phabricator instance.

[BasicAA] Don't assume tail calls with byval don't alias allocas
ClosedPublic

Authored by rnk on Aug 13 2018, 4:45 PM.

Details

Summary

Calls marked 'tail' cannot read or write allocas from the current frame
because the current frame might be destroyed by the time they run.
However, a tail call may use an alloca with byval. Calling with byval
copies the contents of the alloca into argument registers or stack
slots, so there is no lifetime issue.

Fixes PR38466, a longstanding bug.

Event Timeline

rnk created this revision.Aug 13 2018, 4:45 PM

The DSE test is fine, and I think that you should keep it, but can you please add a direct AA test (like, e.g., test/Analysis/BasicAA/cs-cs.ll)?

Should we just always return ModRefInfo::Ref in this case? It can read the data (to copy it), but can't write, correct?

rnk added a comment.Aug 13 2018, 5:39 PM

The DSE test is fine, and I think that you should keep it, but can you please add a direct AA test (like, e.g., test/Analysis/BasicAA/cs-cs.ll)?

Should we just always return ModRefInfo::Ref in this case? It can read the data (to copy it), but can't write, correct?

I thought about that, but I think the code below this check is usually more precise in these cases. It tests if the alloca isNonEscapingLocalObject and then looks at each argument to see if it is actually used by the call. For local objects that *are* escaping, we could return ModRefInfo::Ref. Basically, we already get this test case:

define void @dead(i32* byval %x) {
entry:
  %p = alloca i32
  %q = alloca i32
  %v = load i32, i32* %x
  store i32 %v, i32* %p
  store i32 1, i32* %q  ; DEAD
  tail call void @g(i32* byval %p)
  store i32 2, i32* %q
  call void @escape(i32* %q)
  ret void
}
hfinkel accepted this revision.Aug 13 2018, 5:42 PM
In D50679#1198248, @rnk wrote:

The DSE test is fine, and I think that you should keep it, but can you please add a direct AA test (like, e.g., test/Analysis/BasicAA/cs-cs.ll)?

Should we just always return ModRefInfo::Ref in this case? It can read the data (to copy it), but can't write, correct?

I thought about that, but I think the code below this check is usually more precise in these cases. It tests if the alloca isNonEscapingLocalObject and then looks at each argument to see if it is actually used by the call. For local objects that *are* escaping, we could return ModRefInfo::Ref. Basically, we already get this test case:

define void @dead(i32* byval %x) {
entry:
  %p = alloca i32
  %q = alloca i32
  %v = load i32, i32* %x
  store i32 %v, i32* %p
  store i32 1, i32* %q  ; DEAD
  tail call void @g(i32* byval %p)
  store i32 2, i32* %q
  call void @escape(i32* %q)
  ret void
}

Ah. Good point.

Please add a direct AA test case. Otherwise, LGTM.

This revision is now accepted and ready to land.Aug 13 2018, 5:42 PM
rnk updated this revision to Diff 160499.Aug 13 2018, 6:12 PM
  • add tests, make it ref only
rnk updated this revision to Diff 160500.Aug 13 2018, 6:16 PM
  • nevermind, be precise
rnk added a comment.Aug 13 2018, 6:18 PM

Ah. Good point.

Please add a direct AA test case. Otherwise, LGTM.

The BasicAA test case revealed that CallSite::onlyReadsMemory doesn't check for byval, but it probably could. That's probably a big change that should be committed separately. I added the test with a FIXME that we should treat byval call arg uses as readonly.

This revision was automatically updated to reflect the committed changes.