[PATCH] D104599: [WIP][Attributor] Derive AAFunctionReachability attribute.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8344
+    }
+    return false;
+  }
----------------
kuter wrote:
> jdoerfert wrote:
> > Couldn't we add dependences here, maybe keep track of all the AAs above and then add dependences in the false case. removes complexity I think
> > 
> This is just for checking a single function. In the `updateImpl` method above we have a for loop that goes over unreachable queries.
> moving the call to  `recieveUpdates` here  would cause us to call it multiple times which not needed.
The concept of "receiveUpdates" seems to be some optimization but I fear it will simply lead to errors.

Let's talk this through:
Adding a dependence is pretty cheap.
We should add dependences as we use AAs not at some point later somehow.
In this function we know what AAs we used to come up with a optimistic result, if any.
I fail to see what would be worse if we directly add dependences to those AAs before we return false, it certainly will make this easier to read as multiple implicit dependences will go away.

(FWIW, if you argue it's an optimization you might have to show how much it saves actually. I expect the result here to be well withing the noise so it's not worth making the code harder to read.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104599