This is an archive of the discontinued LLVM Phabricator instance.

GlobalsAA: InaccessibleMemOnly does not mean ReadNone.
AbandonedPublic

Authored by vaivaswatha on Dec 19 2015, 12:56 AM.

Details

Reviewers
jmolloy
hfinkel
Summary

In AnalyzeCallGraph, if we treat InaccessibleMemOnly the same as ReadNone,
the function will end up having "FMRB_DoesNotAccessMemory". This will lead
to situations such as LICM moving out / eliminating calls such as malloc
(in the future, since malloc does not have InaccessibleMemOnly set yet)
from loops.
Instead, treat InaccessibleMemOnly such that the analysis still maintains
info for the function, but keeps it conservative as ModRef.

Depends on D15605

Diff Detail

Event Timeline

vaivaswatha retitled this revision from to GlobalsAA: InaccessibleMemOnly does not mean ReadNone..
vaivaswatha updated this object.
vaivaswatha added reviewers: jmolloy, hfinkel.
vaivaswatha added a subscriber: llvm-commits.
reames added a subscriber: reames.Jan 4 2016, 5:02 PM

I'm really not sure inaccessible memonly belongs in GlobalsAA at all. Particularly since we haven't added even basis cases to BasicAA.

In general, I think we need to be really careful when implementing this. It's fair to say that the call doesn't modify or read any *particular* LLVM value, but I'm not sure our aliasing model is correct if we say that it can't read *any* LLVM value. In particular, as you point out, inaccessiblememonly may still be modifying memory and have memory dependence.

lib/Analysis/GlobalsModRef.cpp
519

Removing the check here is definitely called for.

528

Adding it here may not be. The problem here is that we need to model the memory dependence between two inaccessablememonly functions. I think the result is correct, but the comment is definitely incorrect for inaccessible memory.

test/Analysis/GlobalsModRef/modreftest.ll
1 ↗(On Diff #43300)

Please separate this into it's own test file since it relies on speculation rather than forwarding to illustrate problems.

vaivaswatha updated this revision to Diff 43956.Jan 4 2016, 6:33 PM

Updating comment and test case as per review comment.

I'm really not sure inaccessible memonly belongs in GlobalsAA at all. Particularly since we haven't added even basis cases to BasicAA.

In general, I think we need to be really careful when implementing this. It's fair to say that the call doesn't modify or read any *particular* LLVM value, but I'm not sure our aliasing model is correct if we say that it can't read *any* LLVM value. In particular, as you point out, inaccessiblememonly may still be modifying memory and have memory dependence.

I understand we need to be careful while using this attribute. So far, GlobalsModRef is the only analysis that actually uses it. "inaccessiblememonly" does access memory (not visible in the module) and it does cause memory dependence. However, this dependence is not ignored. Adding the check (in this change) just ensures that GlobalsAA maintains info for the function and does not immediately discard it. I had mistakenly added it in the other place in an earlier commit and am correcting that now.

reames added a comment.Jan 5 2016, 2:36 PM

Let me repeat my early comment in a slightly stronger form. Unless you have strong evidence that this needs to be in GlobalsAA, please start by implementing this in BasicAA. This will get you most of the properties you're looking for (value forwarding), without the complexity involved in GlobalAA. In particular, teach getModRefInfo(CS, Location) about it.

I also believe that the original GlobalAA patch should be reverted rather than patched in this manner. It was flat out wrong.

hfinkel edited edge metadata.Jan 5 2016, 2:57 PM

Let me repeat my early comment in a slightly stronger form. Unless you have strong evidence that this needs to be in GlobalsAA, please start by implementing this in BasicAA. This will get you most of the properties you're looking for (value forwarding), without the complexity involved in GlobalAA. In particular, teach getModRefInfo(CS, Location) about it.

I also believe that the original GlobalAA patch should be reverted rather than patched in this manner. It was flat out wrong.

I agree with Philip. We need to teach BasicAA about the attribute; only then, if necessary, should we teach GlobalsAA about it. Please revert the GlobalsAA patch, and then, propose a new clean patch for GlobalsAA, if necessary, after the BasicAA work is done.

vaivaswatha abandoned this revision.Jan 6 2016, 5:37 AM