[PATCH] D29897: [BypassSlowDivision] Use ValueTracking to simplify run-time checks

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D29897#684734, @n.bozhenov wrote:

> Well, it might make sense to write opt tests here... But we already have got 3 files with 18 llc tests for division bypassing. Moreover, while working on this patchset, the tests were thoroughly reviewed, it was insisted that the test should be written this way, I had to make a number of commits specifically to modify the tests. So, I don't think it would be a good idea to throw away all the work that has been done and re-write the tests from scratch now.


First, a note about x86 codegen testing: it's correct that the FileCheck script often leaves registers hard-coded and includes labels and other gunk like 'kill' comments. Yes, this makes the tests more fragile, but in practice there's been little to complain about vs. the benefits of tighter checks and ease of updating via script. We could certainly improve the script, but there hasn't been much motivation for that AFAIK.

There are good arguments for both continuing as x86 tests and including cleaner IR tests for CGP. When in doubt, do both? :)
Note that we have a script (for generating opt FileChecks too, so this is easy. I strongly recommend using that if you add IR tests for the same reasons that we prefer to script the x86 codegen checks.

Here's what that opt FileCheck script output looks like for the tests added in this patch (without applying this patch). So I would add these tests in a pre-commit, apply this patch, run the script again, and we just show the IR diffs when this patch lands:

  ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
  ; RUN: opt < %s -codegenprepare -S -mtriple=x86_64-unknown-unknown    | FileCheck %s
  
  define i64 @Test_no_bypassing(i32 %a, i64 %b) nounwind {
  ; CHECK-LABEL: @Test_no_bypassing(
  ; CHECK-NEXT:    [[A_1:%.*]] = zext i32 [[A:%.*]] to i64
  ; CHECK-NEXT:    [[A_2:%.*]] = sub i64 -1, [[A_1]]
  ; CHECK-NEXT:    [[RES:%.*]] = srem i64 [[A_2]], [[B:%.*]]
  ; CHECK-NEXT:    ret i64 [[RES]]
  ;
    %a.1 = zext i32 %a to i64
    ; %a.2 is always negative so the division cannot be bypassed.
    %a.2 = sub i64 -1, %a.1
    %res = srem i64 %a.2, %b
    ret i64 %res
  }
  
  ; No OR instruction is needed if one of the operands (divisor) is known
  ; to fit into 32 bits.
  define i64 @Test_check_one_operand(i64 %a, i32 %b) nounwind {
  ; CHECK-LABEL: @Test_check_one_operand(
  ; CHECK-NEXT:    [[B_1:%.*]] = zext i32 [[B:%.*]] to i64
  ; CHECK-NEXT:    [[RES:%.*]] = sdiv i64 [[A:%.*]], [[B_1]]
  ; CHECK-NEXT:    ret i64 [[RES]]
  ;
    %b.1 = zext i32 %b to i64
    %res = sdiv i64 %a, %b.1
    ret i64 %res
  }
  
  ; If both operands are known to fit into 32 bits, then replace the division
  ; in-place without CFG modification.
  define i64 @Test_check_none(i64 %a, i32 %b) nounwind {
  ; CHECK-LABEL: @Test_check_none(
  ; CHECK-NEXT:    [[A_1:%.*]] = and i64 [[A:%.*]], 4294967295
  ; CHECK-NEXT:    [[B_1:%.*]] = zext i32 [[B:%.*]] to i64
  ; CHECK-NEXT:    [[RES:%.*]] = udiv i64 [[A_1]], [[B_1]]
  ; CHECK-NEXT:    ret i64 [[RES]]
  ;
    %a.1 = and i64 %a, 4294967295
    %b.1 = zext i32 %b to i64
    %res = udiv i64 %a.1, %b.1
    ret i64 %res
  }
  
  ; In case of unsigned long division with a short dividend,
  ; the long division is not needed any more.
  define i64 @Test_special_case(i32 %a, i64 %b) nounwind {
  ; CHECK-LABEL: @Test_special_case(
  ; CHECK-NEXT:    [[A_1:%.*]] = zext i32 [[A:%.*]] to i64
  ; CHECK-NEXT:    [[DIV:%.*]] = udiv i64 [[A_1]], [[B:%.*]]
  ; CHECK-NEXT:    [[REM:%.*]] = urem i64 [[A_1]], [[B]]
  ; CHECK-NEXT:    [[RES:%.*]] = add i64 [[DIV]], [[REM]]
  ; CHECK-NEXT:    ret i64 [[RES]]
  ;
    %a.1 = zext i32 %a to i64
    %div = udiv i64 %a.1, %b
    %rem = urem i64 %a.1, %b
    %res = add i64 %div, %rem
    ret i64 %res
  }


https://reviews.llvm.org/D29897