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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:779
+        !Call->hasFnAttr(Attribute::InaccessibleMemOnly) &&
+        !Call->hasFnAttr(Attribute::InaccessibleMemOrArgMemOnly))))
     if (const Function *F = Call->getCalledFunction())
----------------
nikic wrote:
> ChuanqiXu wrote:
> > nikic wrote:
> > > Do we lose anything substantial with just the `Call->getFunction()->isPresplitCoroutine()` condition?
> > > 
> > > Alternatively, I would implement this as a fixup afterwards that looks something like this:
> > > 
> > > ```
> > > if (Call->getFunction()->isPreSplitCoroutine())
> > >   Min = FunctionModRefBehavior(Min | FMRB_OnlyReadsMemory);
> > > ```
> > For,
> > 
> > ```
> > define void @f() presplitcoroutine  {
> >       entry:
> >         %ArgMemOnlyCall = call i32 @argmemonly_func()
> >         ret void
> >       }
> > 
> >       declare i32 @argmemonly_func() argmemonly
> > ```
> > 
> > The current implementation would get `FMRB_OnlyAccessesArgumentPointees` for `ArgMemOnlyCall`. But the suggested change would get `FMRB_UnknownModRefBehavior`. I am OK with the suggested change if you feel like the benefit is not worth for the cost. I know compilation time is an important feature of Clang/LLVM.
> But isn't FMRB_OnlyAccessesArgumentPointees incorrect, because the thread ID is not an argument, so it accesses non-argument memory?
Oh, I missed that case mentioned by @nhaehnle. Then the current style would return `FMRB_UnknownModRefBehavior ` for  `ReadOnlyCall` but the previous style would return `FMRB_OnlyReadsMemory`.
```
define void @f() presplitcoroutine  {
      entry:
        %ReadOnlyCall = call i32 @readonly_func()
        ret void
}

declare i32 @ readonly_func() readonly
```

But I feel like the benefit is not worthy. So I follow your suggestion in this revision.


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

https://reviews.llvm.org/D127383