Skip to content
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

[GuardUtils] Revert llvm::isWidenableBranch change #66411

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

aleks-tmb
Copy link
Contributor

@aleks-tmb aleks-tmb commented Sep 14, 2023

In the d6e7c16 was introduced util to to extract widenable conditions from branch. That util was applied in the llvm::isWidenableBranch to check if branch is widenable. So we consider branch is widenable if it has widenable condition anywhere in the condition tree. But that will be true when we finish GuardWidening reworking from branch widening to widenable conditions widening.
For now we still need to check that widenable branch is in the form of: br(widenable_condition & (...)),
because that form is assumed by LoopPredication and GuardWidening algorithms.

Fixes: #66418

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 14, 2023

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Changes In the d6e7c16 was introduced util to to extract widenable conditions from branch. That util was applied in the llvm::isWidenableBranch to check if branch is widenable. So we consider branch is widenable if it has widenable condition anywhere in the condition tree. But that will be true when we finish GuardWidening reworking from branch widening to widenable conditions widening. For now we still need to check that widenable branch is in the form of: br(widenable_condition & (...)) -- Full diff: https://github.com//pull/66411.diff

2 Files Affected:

  • (modified) llvm/lib/Analysis/GuardUtils.cpp (+4-1)
  • (modified) llvm/test/Transforms/LoopPredication/basic_widenable_branch_guards.ll (+25)
diff --git a/llvm/lib/Analysis/GuardUtils.cpp b/llvm/lib/Analysis/GuardUtils.cpp
index 83734b7eac0c823..b872286fb939618 100644
--- a/llvm/lib/Analysis/GuardUtils.cpp
+++ b/llvm/lib/Analysis/GuardUtils.cpp
@@ -24,7 +24,10 @@ bool llvm::isWidenableCondition(const Value *V) {
 }
 
 bool llvm::isWidenableBranch(const User *U) {
-  return extractWidenableCondition(U) != nullptr;
+  Value *Condition, *WidenableCondition;
+  BasicBlock *GuardedBB, *DeoptBB;
+  return parseWidenableBranch(U, Condition, WidenableCondition, GuardedBB,
+                              DeoptBB);
 }
 
 bool llvm::isGuardAsWidenableBranch(const User *U) {
diff --git a/llvm/test/Transforms/LoopPredication/basic_widenable_branch_guards.ll b/llvm/test/Transforms/LoopPredication/basic_widenable_branch_guards.ll
index cad4fcec5c81abb..d68279cc7d0b937 100644
--- a/llvm/test/Transforms/LoopPredication/basic_widenable_branch_guards.ll
+++ b/llvm/test/Transforms/LoopPredication/basic_widenable_branch_guards.ll
@@ -2144,6 +2144,31 @@ exit:                                             ; preds = %guarded, %entry
   ret i32 %result
 }
 
+; TODO: Support widenable branch in the form of br((wc and cond0) and cond1)
+define i32 @hoge(i1 %cond0, i1 %cond1, i32 %limit) {
+entry:
+  %wc = call i1 @llvm.experimental.widenable.condition()
+  %and0 = and i1 %wc, %cond0
+  %and1 = and i1 %and0, %cond1
+  br i1 %and1, label %loop, label %deopt
+
+loop:
+  %iv = phi i32 [ %iv.next, %guarded ], [ 0, %entry ]
+  %guard.cond = icmp sgt i32 %iv, 100
+  br i1 %guard.cond, label %deopt, label %guarded
+
+guarded:
+  %iv.next = add i32 %iv, 1
+  %exit.cond = icmp ult i32 %iv, %limit
+  br i1 %exit.cond, label %loop, label %exit
+
+deopt:
+  %deoptcall = call i32 (...) @llvm.experimental.deoptimize.i32(i32 9) [ "deopt"() ]
+  ret i32 %deoptcall
+exit:
+  ret i32 0
+}
+
 declare i32 @llvm.experimental.deoptimize.i32(...)
 
 ; Function Attrs: inaccessiblememonly nounwind

@annamthomas
Copy link
Contributor

For now we still need to check that widenable branch is in the form of: br(widenable_condition & (...))

I think it will be clearer to state why we need to do this check either in the commit message or as a statement in the test added.

@aleks-tmb
Copy link
Contributor Author

For now we still need to check that widenable branch is in the form of: br(widenable_condition & (...))

I think it will be clearer to state why we need to do this check either in the commit message or as a statement in the test added.

Done

In the d6e7c16 was introduced util to
to extract widenable conditions from branch. That util was applied in
the llvm::isWidenableBranch to check if branch is widenable. So we
consider branch is widenable if it has widenable condition anywhere in
the condition tree. But that will be true when we finish GuardWidening
reworking from branch widening to widenable conditions widening.

For now we still need to check that widenable branch is in the form of:
`br(widenable_condition & (...))`
because that form is assumed by LoopPredication and GuardWidening
algorithms.

Fixes: llvm#66418
@annamthomas
Copy link
Contributor

Thanks. LGTM.

@aleks-tmb aleks-tmb merged commit e9dfe08 into llvm:main Sep 20, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LoopPredication] Wrong widenable branch identification
3 participants