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

[LoopPredication] Wrong widenable branch identification #66418

Closed
aleks-tmb opened this issue Sep 14, 2023 · 0 comments · Fixed by #66411
Closed

[LoopPredication] Wrong widenable branch identification #66418

aleks-tmb opened this issue Sep 14, 2023 · 0 comments · Fixed by #66411
Assignees

Comments

@aleks-tmb
Copy link
Contributor

We met a testcase with a widenable branch in the form of br(c0 && (c1 && WC)):

  %wc = call i1 @llvm.experimental.widenable.condition()
  %and0 = and i1 %wc, %cond0
  %and1 = and i1 %and0, %cond1
  br i1 %and1, label %loop, label %deopt

That form is going to be supported after we finish GuardWidening reworking from branch widening to widenable conditions widening. But for now we still need to check that widenable branch is in the form of:

br(widenable_condition & (...))

That's why the one of the changes in the d6e7c16 was introduced too early:

 bool llvm::isWidenableBranch(const User *U) {
-  Value *Condition, *WidenableCondition;
-  BasicBlock *GuardedBB, *DeoptBB;
-  return parseWidenableBranch(U, Condition, WidenableCondition, GuardedBB,
-                              DeoptBB);
+  return extractWidenableCondition(U) != nullptr;
 }

We need to revert that change to state that branch above isn't widenable.

@aleks-tmb aleks-tmb self-assigned this Sep 14, 2023
aleks-tmb pushed a commit to aleks-tmb/llvm-project that referenced this issue Sep 15, 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: llvm#66418
aleks-tmb added a commit that referenced this issue Sep 20, 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

Co-authored-by: Aleksander Popov <apopov@azul.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants