[PATCH] D155956: [FunctionAttrs] Consider recursive argmem effects (PR63936)

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:279
+  if (ArgMR != ModRefInfo::NoModRef)
+    ME |= RecursiveArgME & MemoryEffects(ArgMR);
+
----------------
goldstein.w.n wrote:
> nikic wrote:
> > goldstein.w.n wrote:
> > > I don't really understand the rationale behind creating a seperate recursive variable.
> > > 
> > > It seems the original intent is to track calls with bundled arguments for which apprently IRMemLocation is incomplete.
> > > But if you only add back recursive args if memloc indicates mod/ref, doesn't that defeat the purpose?
> > > If so, why not just update ME directly for bundled calls?
> > Lets take a case like this:
> > ```
> > define void @test_recursive_argmem_read(ptr %p) {
> >   %pval = load ptr, ptr %p
> >   call void @test_recursive_argmem_read(ptr %pval)
> >   ret void
> > }
> > ```
> > At the point where we inspect the recursive `@test_recursive_argmem_read` call, we don't know whether that call will read or write `%pval`. (Well, in this specific example we do if we scan instructions from top to bottom, but generally we don't, because it depends on visitation order.)
> > 
> > We could just conservatively assume that all pointers passed to a recursive access will be read+write accessed and add the corresponding effects right away. That would be correct, but it would also be overly conservative. In this case, it is sufficient to only add read effects for the affected locations, we don't need to add any write effects.
> But why do you need a seperate variable to track that? Can't that just as easily be added directly to ME?
I don't really understand what you have in mind here. The current code does `AddArgLocs(RecursiveArgME, ModRefInfo::ModRef)`, what would I replace it with?

I can replace it with `AddArgLocs(ME, ModRefInfo::ModRef)`, but that would add unnecessary effects if the SCC is not `argmem: readwrite`.


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

https://reviews.llvm.org/D155956