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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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

> In D142292#4071665 <https://reviews.llvm.org/D142292#4071665>, @nikic wrote:
>
>> What I didn't really understand are your examples in the patch description. The referenced godbolt examples don't contain select instructions, and the input program also doesn't look like it should involve selects, so I'm a bit unclear on the relation these examples have to the patch.
>
> I think non-refactoring patches that say what they do, but don't say why they do what they do, should be automatically reverted.
> That TLDR is my general motivation. Sure, they no longer have `select`'s because they got canonicalized into min/max, but that isn't true for everything.

Can you update your examples to use a case that involves selects? The current ones are more confusing than helpful as far as motivation is concerned.



================
Comment at: llvm/include/llvm/Analysis/ScalarEvolutionExpressions.h:923
+      NewOp = ((SC *)this)->visit(Op);
+      Changed |= Op != NewOp;
+    }
----------------
FWIW I find this kind of std::array + zip + std::get + references code hard to understand. Maybe an STLExtras helper for this recurring pattern (initialize an std::array from a range) would be useful. Though personally I'd just write this using SmallVector, just like all the other methods above. The additional complication of std::array isn't worthwhile for code that is not performance sensitive.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:4395
+  if (const SCEV *S = findExistingSCEVInCache(Kind, Ops))
+    return S;
+
----------------
This check can currently be omitted, because we already check for existing scev below. This early check is only needed if there are expensive recursive folds which we try to bypass.


================
Comment at: llvm/test/Analysis/ScalarEvolution/exit-count-select-safe.ll:577
 ; CHECK-NEXT:    %cond = select i1 %cond_p0, i1 %cond_p1, i1 false
-; CHECK-NEXT:    --> (%cond_p0 umin %cond_p1) U: full-set S: full-set Exits: <<Unknown>> LoopDispositions: { %loop: Variant }
+; CHECK-NEXT:    --> (select %cond_p0, %cond_p1, false) U: full-set S: full-set Exits: <<Unknown>> LoopDispositions: { %loop: Variant }
 ; CHECK-NEXT:  Determining loop execution counts for: @logical_and_implies_poison1
----------------
Regression, here and below. I think the regression itself isn't even particularly important, more that this loses most of our test coverage for implied poison reasoning in SCEV.


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/update-scev.ll:15
 ; SCEV: Loop %inner_loop_begin: <multiple exits> Unpredictable backedge-taken count.
-; SCEV: Loop %outer_loop_begin: Unpredictable backedge-taken count.
+; SCEV: Loop %outer_loop_begin: Predicated backedge-taken count is (-1 + (1 smax %n))<nsw>
 ;
----------------
Why is this matching the predicated count?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142292