[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:192
+        // turns out to access argmem.
+        addArgLocs(RecursiveArgME, Call, ModRefInfo::ModRef, AAR);
         continue;
----------------
goldstein.w.n wrote:
> goldstein.w.n wrote:
> > Why is arg3 `ModRefInfo::ModRef` and not `AAR.getMemoryEffects(Call).getModRef(IRMemLocation::ArgMem)`?
> Note: you could change this codes to:
> ```
>         MemoryEffects RecursiveArgME = MemoryEffects::none();
>         ModRefInfo ArgMR = ME.getModRef(IRMemLocation::ArgMem);
>         addArgLocs(RecursiveArgME, Call, ModRefInfo::ModRef, AAR);
>         if (ArgMR != ModRefInfo::NoModRef)
>           ME |= RecursiveArgME & MemoryEffects(ArgMR);
> 
> ```
> 
> and get rid of the returning pair logic.
> Personally think thats cleaner but not particularly important.
That's a complicated way of saying `addArgLocs(ME, Call, ME.getModRef(IRMemLocation::ArgMem), AAR)`. The problem is that this uses the `ME` at the current point of the analysis -- however, additional effects may be added later (either in this function, or a different function in the same SCC), which we would fail to account for.


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

https://reviews.llvm.org/D155956