[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:145
 
-  auto AddLocAccess = [&](const MemoryLocation &Loc, ModRefInfo MR) {
+  auto AddLocAccess = [&](MemoryEffects &ME, const MemoryLocation &Loc,
+                          ModRefInfo MR) {
----------------
Imo `ME` needs a different name, its extremely confusing to have it shadow the non-scoped version.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:170
     if (auto *Call = dyn_cast<CallBase>(&I)) {
-      // Ignore calls to functions in the same SCC, as long as the call sites
-      // don't have operand bundles.  Calls with operand bundles are allowed to
-      // have memory effects not described by the memory effects of the call
-      // target.
+      auto AddArgLocs = [&](MemoryEffects &ME, ModRefInfo ArgMR) {
+        for (const Use &U : Call->args()) {
----------------
Imo `ME` needs a different name, its extremely confusing to have it shadow the non-scoped version.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:279
+  if (ArgMR != ModRefInfo::NoModRef)
+    ME |= RecursiveArgME & MemoryEffects(ArgMR);
+
----------------
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?


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

https://reviews.llvm.org/D155956