[PATCH] D130728: [SCEV] Iteratively compute ranges for deeply nested expressions.

Tue Aug 2 01:38:38 PDT 2022

fhahn added a comment.

In D130728#3691284 <https://reviews.llvm.org/D130728#3691284>, @nikic wrote:

> High-level concern: We have lots of code that is working with SCEVs recursively. If we fix getRangeRef(), are we just shifting the problem over to GetMinTrailingZeroes(), or getSCEVAtScope(), or ...?

That's true, I think we have another known crash due to recursion in `getSCEVAtScope`, `GetMinTrailingZeroes` also looks like a likely candidate for issues. As far as I know there are no other reported crashes caused by stack overflows other than the one in `getRangeRef` and `getSCEVAtScope`.

The `getRangeRef` situation is particularly unfortunate, because not only are we traversing operands of a single SCEV expression tree, but we also look through SCEVUnknown phis.

> I wonder whether it would make sense to prevent the creation of very deeply nested SCEVs instead. We already have various limits, and the one that is most relevant is probably the huge expression limit, but that one limits the total number of (recursive) children, not the nesting.

A limit on the nesting may help in some (most?) cases, but one potential drawback would be that SCEVs may depend on the order they have been constructed (e.g. if we start constructing form the bottom of the tree we may miss folds for expressions higher up in the tree that may reduce overall height; if we construct from the top first we could end up with a different expression). It probably won't matter in too many cases in practice, but at least conceptually avoiding hard limits seems desirable if possible IMO. Updating most or all places to work iteratively if needed is probably going to take longer and more work, but so far the changes have been fairly targeted/isolated I think.

As mentioned earlier, `getRangeRef` looks through `SCEVUnknown` phis so it may traverse multiple SCEV expressions, so a limit on the size of a single expression may not help in all cases there.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130728