[PATCH] D118090: [ScalarEvolution] Handle <= and >= in non infinite loops

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added a comment.

I am also getting performance degradation from this patch. The backedge count with the `max` is considered as high cost and it blocks to rewrite exit value in IndVarSimplify pass. It affects LSR's decision to rewrite IV and it causes additional instructions in loop...
As far as I understand, the expression of backedge count describes `(max(End,Start)-Start)/Stride` normally. If the expression satisfies with `(max(RHS,Start) > Start - Stride`, the expression is refined simply.
In order to prove `(max(RHS,Start) > Start - Stride`, SCEV is using below code.

  // Can we prove (max(RHS,Start) > Start - Stride?
  if (isLoopEntryGuardedByCond(L, Cond, OrigStartMinusStride, OrigStart) &&
      isLoopEntryGuardedByCond(L, Cond, OrigStartMinusStride, OrigRHS)) {

Let's see the reduced example of @nikic on https://github.com/llvm/llvm-project/issues/54191 with above code.

  ; RUN: opt -S -passes='print<scalar-evolution>' < %s
  define void @test(i64 %n) mustprogress {
  entry:
    %guard = icmp sgt i64 %n, 1
    br i1 %guard, label %loop, label %exit
    
  loop:
    %iv = phi i64 [ 2, %entry ], [ %iv.next, %loop ]
    %iv.next = add nuw nsw i64 %iv, 2
    %cmp = icmp sle i64 %iv.next, %n
    br i1 %cmp, label %loop, label %exit
    
  exit:
    ret void
  } 

With this patch, the second `isLoopEntryGuardedByCond` is failed because the loop condition's upper bound has been changed from `%n` to `%n + 1` and it is over the guard condition's upper bound which is `%n`.
Without this patch, on `SimplifyICmpOperands`, the lower bound is changed from `4` to `3`because it is not minimum signed value and upper bound is maximum signed value.
At this point, I have questions.

1. Can we check LHS first on `SimplifyICmpOperands` as below?

      if (!getSignedRangeMin(LHS).isMinSignedValue()) {
  ...
      } else if (ControllingFiniteLoop || !getSignedRangeMax(RHS).isMaxSignedValue()) {
  ...
      }

2. Can we change the default value of `SCEVCheapExpansionBudget` to bigger value?
  - The smax's cost checks the cost of `Instruction::ICmp` and `Instruction::Select`. The default value 4 could not be good enough for the smax.
3. If the upper bound is signed maximum signed value, the upper bound change with `+1` could cause overflow and `isLoopEntryGuardedByCond` returns false...
  - It could be safe to check just maximum signed value of upper bound `!getSignedRangeMax(RHS).isMaxSignedValue()` without `ControllingFiniteLoop `.

How do you think about it? If I missed something, please let me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118090