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

[Kuter Dinel via Phabricator via llvm-commits]

kuter added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8344
+    }
+    return false;
+  }
----------------
jdoerfert wrote:
> 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.)
I agree that moving the contentes of `receiveUpdates` here should be ok since adding deps is fast.
I fear that things can get really slow if we are not careful about our dependencies.
Since we are doing this in a recursive way we are actually creating a lot of `AAFunctionReachability` attributes.

if all functions that where queried where reachable, we do not need any dependencies since functions can't become unreachable after we find that they are reachable. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104599