[PATCH] D114650: [SCEV] Construct SCEV iteratively (WIP).

Mon Nov 29 02:55:59 PST 2021

fhahn added a comment.

In D114650#3156273 <https://reviews.llvm.org/D114650#3156273>, @lebedev.ri wrote:

> Thanks for looking into this!
> I previously looked into this: https://github.com/LebedevRI/llvm-project/commit/6b006aa21caf018aa0f280828899d510274c8444
> ... but as it is apparent, never posted the diff. Nonetheless, it may be interesting to look at.
>
> The most interesting question is, do we really want to avoid (indirect self-) recursion (through getSCEV)?
> Then the api of ScalarEvolution needs more changes than what the current diff contains,
> basically everything that can end up creating new SCEV expressions from IR
> needs to be able to return "in-progress, requery me later" status.

Thanks for sharing the patch. I think the current patch doesn't really need to change any existing interfaces, it just introduces a new expectation: before creating a SCEV for `I`,  SCEVs for all of `I`'s operands must already exist. It's up to the callers of `createSCEV` (and friends) to ensure that. The only additional complexity is 1) extracting operands for which we need to create SCEVs and 2) processing them in the right order.

At the moment, 1) is a bit unfortunate, because it requires some duplication of `createSCEV` logic, but overall it's not too bad (if no other interfaces need modifying IMO). I think we may be able to improve/unify the logic there as well, but I think first it would be good to get agreement on whether we want to handle the issue by constructing SCEVs iteratively or not. (Getting this patch to work for all cases, cleaning it up and making sure its as fast as possible will be quite a bit of additional work, which I'd only like to do once we it's clear that this is the overall direction we want to pursue :))

In D114650#3157263 <https://reviews.llvm.org/D114650#3157263>, @lebedev.ri wrote:

> In D114650#3157253 <https://reviews.llvm.org/D114650#3157253>, @nikic wrote:
>
>> To ask the obvious question: Can we get away with simply adding a getSCEV recursion limit instead? Is there reason to believe that a sensible recursion cutoff (similar to the many SCEV already has) would adversely affect analysis quality in practice? I'd rather avoid the additional complexity if we can.
>
> Recursion itself is rarely the right solution in the first place, let alone working around it by introducing random cut-offs.

I think one issue with a cut-off for the recursion is that such a cut-off would make SCEV instantiations depend on the instantiation order. I am not sure how much this would be an issue in practice, but among other things it may make verification of SCEV harder (e.g.  if we compute SCEVs from scratch to the cached version we would have to make sure to use the same evaluation order as when computing the original SCEV).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114650