[PATCH] D116766: [SCEV] Sequential/in-order `UMin` expression

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

I skimmed the code for a sanity check, but am mostly just commenting on the high level bits.

I'm not convinced this is the right approach, but I also don't want to block iterative progress.  My primary concern is that I don't see how this solves the multiple exit case and that I think we're going to end up needing a new representation there, which indirectly solves this problem.  However, I think we can work step-wise here, and come back to refactor/clean this up if needed.

Personally, I think I'd have preferred to introduce a freeze node as that seems potentially more reusable, but I have no strong argument there, just a gut feel.

On naming, I'd suggest ordered reduction as that's the term I'm familiar with from the vectorizaion literature for floating point reductions, and poison in integer domain seems analogous, but this is a fairly minor point.

So overall, weakly hesitant, but don't let that stop you.



================
Comment at: llvm/include/llvm/IR/IRBuilder.h:1574
 
+  // NOTE: this is sequential, non-commutative reduction!
+  Value *CreateLogicalOr(ArrayRef<Value *> Ops) {
----------------
I'd expand this comment a bit.

>From the vectorization literature, the term is often ordered reduction.  Might be good to use the same name.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116766