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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/GlobalsModRef.cpp:270
+            // Since a coroutine might resume in different threads.
+            !Call->getFunction()->isPresplitCoroutine())
           Min = FMRB_DoesNotAccessMemory;
----------------
ChuanqiXu wrote:
> ChuanqiXu wrote:
> > nikic wrote:
> > > I've taken the liberty of deleting this whole function in https://github.com/llvm/llvm-project/commit/4a579abd9f95bf9fda920759aced3874d04c5b9e. This was unnecessary code duplication with BasicAA.
> > This might not be right. After the patch, when I call GlobalsAAResult::getModRefBehavior(CallBase*), it would call https://github.com/llvm/llvm-project/blob/519d7876cbee5a5d3cd40d41525cd45e44fb07a8/llvm/include/llvm/Analysis/AliasAnalysis.h#L1236-L1238 all the time, which is not correct.
> I think we could improve the original implementation. I thought it would call BasicAAResult::getModRefBehavior(CallBase*) too.
> This might not be right. After the patch, when I call GlobalsAAResult::getModRefBehavior(CallBase*), it would call https://github.com/llvm/llvm-project/blob/519d7876cbee5a5d3cd40d41525cd45e44fb07a8/llvm/include/llvm/Analysis/AliasAnalysis.h#L1236-L1238 all the time, which is not correct.

This is fine. GlobalsAAResult methods are not intended to be called directly -- the public alias analysis API goes through an AAResults aggregate over multiple AA providers, which always includes BasicAA in a real pipeline. AA providers should not reimplement functionality provided by other AA providers, they are designed to compose via AAResults instead.


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

https://reviews.llvm.org/D127383