Skip to content

[SCEV] Missing implication opportunity: cannot infer inequality from 2 guarding conditions #58074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
xortator opened this issue Sep 30, 2022 · 2 comments

Comments

@xortator
Copy link
Contributor

xortator commented Sep 30, 2022

It seems that IndVars, through SCEV implication engine, fails to prove some facts if they are implied by two different dominating guards. Consider the following example:

Known facts:

min_len <= len
iv < min_len

Need to prove:

iv < len

Though this is obvious, it seems that the implication engine cannot pull two facts together to prove iv < min_len <= len, which is embarassing.

Motivation test opt -passes=indvars -S:

define i32 @test_01(i32 %start, i32 %len, i32 %min_len) {
entry:
  br label %loop

loop:
  %iv = phi i32 [ %start, %entry ], [ %iv.next, %backedge ]
  %precondition = icmp ule i32 %min_len, %len
  br i1 %precondition, label %guarded_1, label %precondition_failed

guarded_1:
  %check_1 = icmp ult i32 %iv, %min_len
  br i1 %check_1, label %guarded_2, label %check_failed

guarded_2:
  %check_2 = icmp ult i32 %iv, %len
  br i1 %check_2, label %backedge, label %check_failed

backedge:
  %iv.next = add i32 %iv, 1
  %loop_cond = call i1 @cond()
  br i1 %loop_cond, label %loop, label %exit
  
exit:
  ret i32 %iv

precondition_failed:
  unreachable

check_failed:
  ret i32 -1
}


define i32 @test_02(i32 %start, i32 %len, i32 %min_len) {
entry:
  %precondition = icmp ule i32 %min_len, %len
  br i1 %precondition, label %loop, label %precondition_failed

loop:
  %iv = phi i32 [ %start, %entry ], [ %iv.next, %backedge ]
  %check_1 = icmp ult i32 %iv, %min_len
  br i1 %check_1, label %guarded, label %check_failed

guarded:
  %check_2 = icmp ult i32 %iv, %len
  br i1 %check_2, label %backedge, label %check_failed

backedge:
  %iv.next = add i32 %iv, 1
  %loop_cond = call i1 @cond()
  br i1 %loop_cond, label %loop, label %exit
  
exit:
  ret i32 %iv

precondition_failed:
  unreachable

check_failed:
  ret i32 -1
}

declare i1 @cond()
@xortator
Copy link
Contributor Author

xortator commented Sep 30, 2022

We can remove the check in this case:

define i32 @test_03(i32 %start, i32 %len, i32 %min_len) {
entry:
  br label %loop

loop:
  %iv = phi i32 [ %start, %entry ], [ %iv.next, %backedge ]
  %umin.cmp = icmp ult i32 %len, %min_len
  %umin = select i1 %umin.cmp, i32 %len, i32 %min_len
  %check_1 = icmp ult i32 %iv, %umin
  br i1 %check_1, label %guarded, label %check_failed

guarded:
  %check_2 = icmp ult i32 %iv, %len
  br i1 %check_2, label %backedge, label %check_failed

backedge:
  %iv.next = add i32 %iv, 1
  %loop_cond = call i1 @cond()
  br i1 %loop_cond, label %loop, label %exit
  
exit:
  ret i32 %iv

precondition_failed:
  unreachable

check_failed:
  ret i32 -1
}

because it's inferrable from a sole condition.

@fhahn
Copy link
Contributor

fhahn commented Sep 30, 2022

ConstraintElimination can handle cases like this: https://llvm.godbolt.org/z/n5ePssasn

@EugeneZelenko EugeneZelenko added llvm:SCEV Scalar Evolution and removed new issue labels Sep 30, 2022
@fhahn fhahn closed this as completed in fb13dcf Jan 4, 2023
@EugeneZelenko EugeneZelenko added llvm:optimizations and removed llvm:SCEV Scalar Evolution labels Jan 4, 2023
fhahn added a commit that referenced this issue Feb 6, 2023
This reverts commit 695ce48.

The compile-time regression causing the revert has been fixed. Recommit
the original patch.

Original commit message:

   The pass should help to close a functional gap when it comes to
    reasoning about related conditions in a relatively general way.

    It addresses multiple existing issues (linked below) and the need for a
    more powerful reasoning system was also discussed recently in
    https://discourse.llvm.org/t/rfc-alternative-approach-of-dealing-with-implications-from-comparisons-through-pos-analysis/65601/7

    On AArch64, the new pass performs ~2000 simplifications on
    MultiSource,SPEC2006,SPEC2017 with -O3.

    Compile-time impact:

    NewPM-O3: +0.20%
    NewPM-ReleaseThinLTO: +0.32%
    NewPM-ReleaseLTO-g: +0.28%

    https://llvm-compile-time-tracker.com/compare.php?from=f01a3a893c147c1594b9a3fbd817456b209dabbf&to=577688758ef64fb044215ec3e497ea901bb2db28&stat=instructions:u

    Fixes #49344.
    Fixes #47888.
    Fixes #48253.
    Fixes #49229.
    Fixes #58074.

    Reviewed By: asbirlea

    Differential Revision: https://reviews.llvm.org/D135915
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants