[PATCH] D97401: [basicaa] Recurse through a single phi input

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

In D97401#2589132 <https://reviews.llvm.org/D97401#2589132>, @reames wrote:

> In D97401#2585880 <https://reviews.llvm.org/D97401#2585880>, @nikic wrote:
>
>> I'm rather leery of relaxing this until we have a reliably complexity limit for BasicAA (D96647 <https://reviews.llvm.org/D96647> or similar). From past experience, it's very easy to open up new pathological compile-time cases in BasicAA, because it walks a really thin line to avoid exponential queries in the absence of complexity cutoffs.
>
> I understand your hesitancy here, but let me explain why I think this should go forward regardless.  Specifically, with the most restricted form of the patch as updated.
>
> 1. With the updated patch, we specifically only continue analysis when there's a single phi value being recursed on.  Given that, this patch definitely can not create an exponential number of queries.  It can still expose potential exponential that already exist by reaching deeper into the graph, but that's a separate issue.
>
> 2. This is a really critical case.  Specifically, it's the case of a pointer induction variable in a loop nest.  It seems really embarrassing to have LLVM miss this.
>
> 3. I'm general hesitant to block on progress on fixing an exponential issue.  This isn't the only one we have and it isn't the first time we've hit one.  In general, different contributors will be working on different corpus, and what shows in one place doesn't in another.  We certainly don't want to take changes which make a problem markedly worse on anyone's corpus, but something that is orthogonal in practice (if not in theory) seems good to move forward with.

This approach seems like a relatively safe extensions of the PHI handling (in terms of introducing additional queries) to me, while giving some nice improvements. I tried this with -O3 & LTO on X86 and there's positive changes in a couple of benchmarks in terms of additional `noalias` results, improvements to removed stores, unrolling & additional vectorization.

With respect to 3., I would be cautious of introducing new exponential issues. But as you mentioned, I don't see how this patch would introduce new exponential behavior. If it exposes existing exponential behavior, we can revert & focus on fixing the behavior.

LGTM, but it would be good to wait a bit in case @nikic has concerns with the latest version.


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

https://reviews.llvm.org/D97401