[PATCH] D136512: [AA] Add aliasAt, for flow-sensitive queries.

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Some initial notes:

- Rather than adding a separate `aliasAt()` API, we should add an extra optional argument to `alias()`. Especially when it comes to the Concept/Model/Base I don't see a value in having both alias() and aliasAt().
- An important limitation (which your patches do get right) is that BasicAA must not make use of the context instruction, otherwise it would invalidate our caching strategy. This does mean that recursive queries will not be able to benefit from the context instruction (or they would need to use a different context instruction that only depends on the memory locations, which is tricky in itself due to symmetry requirements).
- I don't really understand your invalidation troubles in the new AA pass. Your current implementation has a high compile-time impact (1% geomean on ctmark), but I think this mainly comes down to the fact that you are unnecessarily recalculating the dominator tree many times -- why doesn't this fetch the DT from the pass manager? BasicAA already does this, so it should be available and work fine. (Note that this is not just a performance concern, your current implementation is incorrect if the DT is updated within a pass -- this will update the DT from the PM, but not your internal instance.)
- It might make sense to use an assume operand bundle rather than a separate intrinsic, something like `call void @llvm.assume(i1 true) ["noalias"(ptr %o1, ptr %o2)]`. This could give you various things like ephemeral value skipping for "free". And probably also integrates with AssumptionCache already. Not sure if there's any downsides to this representation.
- The second patch does not apply cleanly, because your base sources seem to register an additional objc AA pass.

Generally, the approach looks viable to me though. To suggest a possible alternative, it should be possible to make some other pass add scoped noalias metadata to accesses based on this intrinsic. Not sure this would be better though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136512