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

Mon Oct 24 14:28:46 PDT 2022

davidtgoldblatt added a comment.

> 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().

Agreed, that's better.

> 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).

Yeah. I think it's possible to extend it in certain cases, but didn't feel sure enough to try it.

> 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.)

I think practically the reason is that BasicAA is still fairly useful without the dominator tree, while SeparateStorageAA is much less so. But a hard DT dependency is somewhat burdensome because any AA result being invalidated acts to invalidate all of them (just because of the invalidate implementation in AAResults). But I think it's just a bullet this has to bite given the correctness/compile-time problems.

> 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.

This is a good idea, I'll do that. This also nicely solves various other annoyances: duplicated special-casing logic, the possibility of failing to notice intrinsic creations due to duplication when cloning, etc.

> The second patch does not apply cleanly, because your base sources seem to register an additional objc AA pass.

Will fix.

> 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.

I did some experiments in this direction; it turned out trickier than I expected because of the impedance mismatch with `noalias` scopes. Example: changes in control flow (inlining, DCE, etc.) can cause a `llvm.experimental.separate.storage` call to start dominating an instruction it previously didn't, but we'd need to re-run a metadata propagation pass for subsequent AA queries to notice. Entirely possible it's just due to me being dumb, but the similarity between the semantics of this and `assume`s make the operand-bundle approach feel right.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136512