Skip to content

Commit

Permalink
[InstCombine] canonicalize branch with logical-and-not condition
Browse files Browse the repository at this point in the history
https://alive2.llvm.org/ce/z/EfHlWN

In the motivating case from issue #58313,
this allows forming a duplicate 'not' op
which then gets CSE'd and simplifyCFG'd
and combined into the expected 'xor'.
  • Loading branch information
rotateright committed Oct 31, 2022
1 parent a7265f9 commit 115d2f6
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 16 deletions.
11 changes: 10 additions & 1 deletion llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
Expand Up @@ -3180,13 +3180,22 @@ Instruction *InstCombinerImpl::visitBranchInst(BranchInst &BI) {

// Change br (not X), label True, label False to: br X, label False, True
Value *Cond = BI.getCondition();
Value *X = nullptr;
Value *X;
if (match(Cond, m_Not(m_Value(X))) && !isa<Constant>(X)) {
// Swap Destinations and condition...
BI.swapSuccessors();
return replaceOperand(BI, 0, X);
}

// br (X && !Y), T, F --> br ((X && Y) || !X), F, T

This comment has been minimized.

Copy link
@nikic

nikic Oct 31, 2022

Contributor

I'm really confused by the RHS of this comment. Did you mean br (!X || Y), F, T?

This comment has been minimized.

Copy link
@rotateright

rotateright Oct 31, 2022

Author Contributor

I'm really confused by the RHS of this comment. Did you mean br (!X || Y), F, T?

Yes, this deserves a better comment. I was trying to show via logic ops what the select equals:
select X, Y, true --> (X && Y) || (!X && true) --> (X && Y) || !X --> !X || Y

So we need to spell this out...
We want to invert the condition and swap the successors, so the inverse condition is:
!(X && !Y) -->
!X || Y --> ; demorgan
!X || (X && Y) --> ; reverse logic reduction
(X && Y) || !X --> ; commute
select X, Y, true ; logic as select

Value *Y;
if (isa<SelectInst>(Cond) &&
match(Cond, m_OneUse(m_LogicalAnd(m_Value(X), m_Not(m_Value(Y)))))) {
Value *AndOr = Builder.CreateSelect(X, Y, Builder.getTrue());

This comment has been minimized.

Copy link
@nikic

nikic Oct 31, 2022

Contributor

This should probably use CreateLogicalOr and an explicit negate -- and because of that, needs a one-use on the m_Not as well?

Generally I'm missing some multi-use tests here, both for extra use on the condition and on the not.

This comment has been minimized.

Copy link
@rotateright

rotateright Oct 31, 2022

Author Contributor

Tests added and updated the code + comments to be hopefully less confusing here:
1f8ac37

BI.swapSuccessors();
return replaceOperand(BI, 0, AndOr);
}

// If the condition is irrelevant, remove the use so that other
// transforms on the condition become more effective.
if (!isa<ConstantInt>(Cond) && BI.getSuccessor(0) == BI.getSuccessor(1))
Expand Down
10 changes: 5 additions & 5 deletions llvm/test/Transforms/InstCombine/branch.ll
Expand Up @@ -113,14 +113,14 @@ merge.2:
ret i1 %merge.cond.2
}

; if (x && !y) ret 42; ret 3
; if (x && !y) ret 42; ret 3 --> if (!x || y) ret 3; ret 42

define i32 @logical_and_not(i1 %x, i1 %y) {
; CHECK-LABEL: @logical_and_not(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[NOTY:%.*]] = xor i1 [[Y:%.*]], true
; CHECK-NEXT: [[AND:%.*]] = select i1 [[X:%.*]], i1 [[NOTY]], i1 false
; CHECK-NEXT: br i1 [[AND]], label [[T:%.*]], label [[F:%.*]]
; CHECK-NEXT: [[NOT_X:%.*]] = xor i1 [[X:%.*]], true
; CHECK-NEXT: [[TMP0:%.*]] = select i1 [[NOT_X]], i1 true, i1 [[Y:%.*]]
; CHECK-NEXT: br i1 [[TMP0]], label [[F:%.*]], label [[T:%.*]]
; CHECK: t:
; CHECK-NEXT: ret i32 42
; CHECK: f:
Expand All @@ -138,7 +138,7 @@ f:
ret i32 3
}

; if (x && y || !x) ret 3; ret 42
; if (x && y || !x) ret 3; ret 42 --> if (!x || y) ret 3; ret 42

define i32 @logical_and_or(i1 %x, i1 %y) {
; CHECK-LABEL: @logical_and_or(
Expand Down
12 changes: 2 additions & 10 deletions llvm/test/Transforms/PhaseOrdering/simplifycfg-options.ll
Expand Up @@ -105,16 +105,8 @@ end:
define i1 @PR58313(i1 %lhs, i1 %rhs) {
; CHECK-LABEL: @PR58313(
; CHECK-NEXT: andandend:
; CHECK-NEXT: [[TMP0:%.*]] = xor i1 [[RHS:%.*]], true
; CHECK-NEXT: [[ANDANDVAL:%.*]] = select i1 [[LHS:%.*]], i1 [[TMP0]], i1 false
; CHECK-NEXT: br i1 [[ANDANDVAL]], label [[OROREND:%.*]], label [[OROR:%.*]]
; CHECK: oror:
; CHECK-NEXT: [[TMP1:%.*]] = xor i1 [[LHS]], true
; CHECK-NEXT: [[ANDANDVAL3:%.*]] = select i1 [[TMP1]], i1 [[RHS]], i1 false
; CHECK-NEXT: br label [[OROREND]]
; CHECK: ororend:
; CHECK-NEXT: [[ORORVAL:%.*]] = phi i1 [ true, [[ANDANDEND:%.*]] ], [ [[ANDANDVAL3]], [[OROR]] ]
; CHECK-NEXT: ret i1 [[ORORVAL]]
; CHECK-NEXT: [[SPEC_SELECT:%.*]] = xor i1 [[LHS:%.*]], [[RHS:%.*]]
; CHECK-NEXT: ret i1 [[SPEC_SELECT]]
;
andandend:
%0 = xor i1 %rhs, true
Expand Down

0 comments on commit 115d2f6

Please sign in to comment.