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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:279
+  if (ArgMR != ModRefInfo::NoModRef)
+    ME |= RecursiveArgME & MemoryEffects(ArgMR);
+
----------------
nikic wrote:
> goldstein.w.n wrote:
> > nikic wrote:
> > > 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`.
> > I'm just wondering why we need a new variable at L171 (as opposed to just doing this all in `ME` on L169).
> > Why would that unduly add `argmen: readwrite`?
> > 
> > Sorry for this to be like pulling teeth, not super familiar with FunctionAttrs.
> > 
> This is what happens if you directly add to ME instead of using a separate variable: https://gist.github.com/nikic/ff98bde042030ce6ebaca392237f7e46
Is there some intuition for why that is?


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

https://reviews.llvm.org/D155956