[PATCH] D89381: [SCEV] Use nw flag and symbolic iteration count to sharpen ranges of AddRecs

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I've reverted this change again for a couple of reasons:

First, the compile-time impact is now better, but still larger than the change justifies (at least without knowing the broader context): https://llvm-compile-time-tracker.com/compare.php?from=fbd62fe60fb2281ca33da35dc25ca3c87ec0bb51&to=32b72c3165bf65cca2e8e6197b59eb4c4b60392a&stat=instructions The mafft regression is no longer 4%, but it's still 3%.

Less importantly, I don't think the logic here is quite correct. The first issue is the use of the unionWith() API, which probably doesn't do what you want it to do. The way it is used here, it will return the smallest union of Start and End, which is not necessarily the range `[Start, End]`, it can also be `Start], [End`, if we simplify this down to single-element ranges. Here is a test-case that I think illustrates the issue, if I didn't mess it up:

  define i32 @test_02(i32 %start, i32* %p, i32* %q) {
  ; CHECK-LABEL: 'test_02'
  ; CHECK-NEXT:  Classifying expressions for: @test_02
  ; CHECK-NEXT:    %zext = zext i32 %start to i64
  ; CHECK-NEXT:    --> (zext i32 %start to i64) U: [0,4294967296) S: [0,4294967296)
  ; CHECK-NEXT:    %shl = shl i64 %zext, 31
  ; CHECK-NEXT:    --> (2147483648 * (zext i32 %start to i64))<nuw><nsw> U: [0,9223372034707292161) S: [0,9223372034707292161)
  ; CHECK-NEXT:    %indvars.iv = phi i64 [ %indvars.iv.next, %loop ], [ %shl, %entry ]
  ; CHECK-NEXT:    --> {(2147483648 * (zext i32 %start to i64))<nuw><nsw>,+,-1}<nsw><%loop> U: [-9223372036854775808,9223372034707292161) S: [-9223372036854775808,9223372034707292161) Exits: -9223372036854775806 LoopDispositions: { %loop: Computable }
  ; CHECK-NEXT:    %indvars.iv.next = add nsw i64 %indvars.iv, -1
  ; CHECK-NEXT:    --> {(-1 + (2147483648 * (zext i32 %start to i64))<nuw><nsw>)<nsw>,+,-1}<nw><%loop> U: full-set S: [-1,-9223372036854775806) Exits: -9223372036854775807 LoopDispositions: { %loop: Computable }
  ; CHECK-NEXT:  Determining loop execution counts for: @test_02
  ; CHECK-NEXT:  Loop %loop: backedge-taken count is (9223372036854775806 + (2147483648 * (zext i32 %start to i64))<nuw><nsw>)<nuw>
  ; CHECK-NEXT:  Loop %loop: max backedge-taken count is -2147483650
  ; CHECK-NEXT:  Loop %loop: Predicated backedge-taken count is (9223372036854775806 + (2147483648 * (zext i32 %start to i64))<nuw><nsw>)<nuw>
  ; CHECK-NEXT:   Predicates:
  ; CHECK:       Loop %loop: Trip multiple is 1
  ;
  entry:
    %zext = zext i32 %start to i64
    %shl = shl i64 %zext, 31
    br label %loop
  
  loop:                                             ; preds = %backedge, %entry
    %indvars.iv = phi i64 [ %indvars.iv.next, %loop ], [ %shl, %entry ]
    %cond = icmp eq i64 %indvars.iv, -9223372036854775806
    %indvars.iv.next = add nsw i64 %indvars.iv, -1
    br i1 %cond, label %exit, label %loop
  
  exit:                                             ; preds = %loop
    ret i32 0
  }

Note that the signed range of `%indvars.iv.next` is `[-1,-9223372036854775806)`, while it should be `[-9223372036854775807,9223372034707292161)`, give or take a few off by one errors.

This could be partially avoided by passing a sign preference to unionWith(). However, even with a sign preference, this may return a smaller wrapping range, if that avoid returning a full set. Similarly, the signed/unsigned ranges you request for start/end are also only sign preferences, you may still get back a wrapping range. I haven't followed it through in detail, but I don't think the logic is correct for wrapping ranges.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89381