[PATCH] D142292: [SCEV] Introduce `SCEVSelectExpr`

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Maybe a naive question, can we model `select cond, x, y` as `zext(cond) * x + zext(1 - cond) * y`, at least when we can prove that `x` and `y` are not poison? Will that help the vectorizer?

I see an interesting prospect of allowing `SCEVCouldNotCompute` for either `TrueValue` or `FalseValue` to model some situations in loop exit counts. Is it worth allowing it?



================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:649
                           bool Sequential = false);
+  const SCEV *getSelectExpr(ArrayRef<const SCEV *> Operands);
   const SCEV *getUnknown(Value *V);
----------------
We don't have such generalized version for div/rem because they have a fixed number of operands, and I also don't think we should have it here, when the number of operands is fixed.


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolutionExpressions.h:561
+
+  std::array<const SCEV *, 3> Operands;
+
----------------
Same as above, don't we want it to be urem-like, with separate fields for semantically separate operands? Any advantage of using an array?


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolutionExpressions.h:566
+    assert(Ops.size() == 3 && "Unexpected operand count!");
+    llvm::copy(Ops, Operands.begin());
+  }
----------------
Maybe some more asserts, same as you have in `getSelectExpr` to make sure these facts hold after all simplifications?


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolutionExpressions.h:581
+
+  Type *getType() const { return getTrueValue()->getType(); }
+
----------------
Is it possible that either TrueVal or FalseVal is SCEVCouldNotCompute? Is it asserted somewhere?

I'm asking because it's not unreasonable to allow it.

Imagine loop:
```
while (isInfinite) { do smth }
```
The exit cound of this loop can be modelled as `select isInfinite, CouldNotCompute, 0`. So allowing this value might be useful. However, in this case, this method's implementation is wrong.

Do we want this? Either yes or no, I'd prefer a clear comment about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142292