[PATCH] D127383: Don't treat readnone call in presplit coroutine as not access memory

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu marked an inline comment as done.
ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:778-780
+  if (Call->hasFnAttr(Attribute::ReadNone) &&
+      Call->getFunction()->isPresplitCoroutine())
+    return FMRB_OnlyReadsMemory;
----------------
jyknight wrote:
> ChuanqiXu wrote:
> > nikic wrote:
> > > ChuanqiXu wrote:
> > > > nikic wrote:
> > > > > ChuanqiXu wrote:
> > > > > > This early return is necessary otherwise it would fall to the combine operation at line 804, which would return FMRB_DoesNotAccessMemory.
> > > > > You mean the getModRefBehavior call on the function below? I think it may be better to guard that call instead.
> > > > Yeah, I mean the call at line 781. I feel it looks better/cleaner/clearer to put the check here. The logic is consistent with the above check (We can't do better.)
> > > With your current implementation, won't you still run into a problem with a writeonly function, which will report as writeonly from function FMRB?
> > Oh, I missed it. Thanks for pointing it out. I'm working on it.
> I don't understand this change. ReadNone/WriteOnly being present on the call versus being present the function doesn't make a difference to the semantics vs presplit coroutines, unlike what's the case for operand bundles.
> 
> Hm...oh, ok...I see why you're doing this. Function::doesNotAccessMemory doesn't (can't) be modified to return false if we're in a coroutine, so we don't get the special-cased behavior there. That divergence is sufficiently confusing that I think we should probably not implement it that way.
> 
> Just looking at the calls to "doesNotAccessMemory", I wonder if we may be better off moving the query to the callers, anyways, instead of modifying Function/Call doesNotAccessMemory (and friends) themselves. AFAICT, it's not actually correct for all of the callers to get the new behavior.
I think it is better to modify `CallBase::doesNotAccessMemory` instead of modify the call sites.

 For the perspective of semantics, we've changed the semantics for `readnone` attribute. It should be naturally correct to modify `Call:: doesNotAccessMemory ` to show the change.

 For the perspective of engineering, I think the current method is better too. What I imaged is a new developer who wants to develop a new optimization pass. The implementation calls `CallBase:: doesNotAccessMemory ` at several places. But the implementation may not be right since he forgets to add the `in presplit coroutine`  checks. What I want to say is that it requires the developers more if we choose to add the check at call sites. But we could avoid it.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127383/new/

https://reviews.llvm.org/D127383