[InstCombine] avoid extra instructions in foldSelectICmpAnd #127398

andjo403 · 2025-02-16T14:32:38Z

Disable fold when it will result in more instructions.

llvmbot · 2025-02-16T14:33:08Z

@llvm/pr-subscribers-llvm-transforms

Author: Andreas Jonson (andjo403)

Changes

Disable fold when it will result in more instructions.

Full diff: https://github.com/llvm/llvm-project/pull/127398.diff

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp (+9-5)
(modified) llvm/test/Transforms/InstCombine/select-icmp-and.ll (+7-12)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 2e14145aef884..3989577ff2667 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -205,11 +205,15 @@ static Value *foldSelectICmpAnd(SelectInst &Sel, ICmpInst *Cmp,
   unsigned ValZeros = ValC.logBase2();
   unsigned AndZeros = AndMask.logBase2();
   bool ShouldNotVal = !TC.isZero();
-
-  // If we would need to create an 'and' + 'shift' + 'xor' to replace a 'select'
-  // + 'icmp', then this transformation would result in more instructions and
-  // potentially interfere with other folding.
-  if (CreateAnd && ShouldNotVal && ValZeros != AndZeros)
+  bool NeedShift = ValZeros != AndZeros;
+  bool NeedZExtTrunc =
+      SelType->getScalarSizeInBits() != V->getType()->getScalarSizeInBits();
+
+  // If we would need to create an 'and' + 'shift' + 'xor' + cast to replace
+  // a 'select' + 'icmp', then this transformation would result in more
+  // instructions and potentially interfere with other folding.
+  if (CreateAnd + ShouldNotVal + NeedShift + NeedZExtTrunc >
+      1 + Cmp->hasOneUse())
     return nullptr;
 
   // Insert the 'and' instruction on the input to the truncate.
diff --git a/llvm/test/Transforms/InstCombine/select-icmp-and.ll b/llvm/test/Transforms/InstCombine/select-icmp-and.ll
index 516a1e8496b43..dbe305a2d336a 100644
--- a/llvm/test/Transforms/InstCombine/select-icmp-and.ll
+++ b/llvm/test/Transforms/InstCombine/select-icmp-and.ll
@@ -391,9 +391,8 @@ define i32 @test15e_extra_use(i32 %X) {
 ;; (a & 128) ? 256 : 0
 define i32 @test15e_zext(i8 %X) {
 ; CHECK-LABEL: @test15e_zext(
-; CHECK-NEXT:    [[TMP1:%.*]] = and i8 [[X:%.*]], -128
-; CHECK-NEXT:    [[TMP2:%.*]] = zext i8 [[TMP1]] to i32
-; CHECK-NEXT:    [[T3:%.*]] = shl nuw nsw i32 [[TMP2]], 1
+; CHECK-NEXT:    [[T2_NOT:%.*]] = icmp sgt i8 [[X:%.*]], -1
+; CHECK-NEXT:    [[T3:%.*]] = select i1 [[T2_NOT]], i32 0, i32 256
 ; CHECK-NEXT:    ret i32 [[T3]]
 ;
   %t1 = and i8 %X, 128
@@ -406,9 +405,7 @@ define i32 @test15e_zext(i8 %X) {
 define i32 @test15e_zext_extra_use(i8 %X) {
 ; CHECK-LABEL: @test15e_zext_extra_use(
 ; CHECK-NEXT:    [[T2:%.*]] = icmp slt i8 [[X:%.*]], 0
-; CHECK-NEXT:    [[TMP1:%.*]] = and i8 [[X]], -128
-; CHECK-NEXT:    [[TMP2:%.*]] = zext i8 [[TMP1]] to i32
-; CHECK-NEXT:    [[T3:%.*]] = shl nuw nsw i32 [[TMP2]], 1
+; CHECK-NEXT:    [[T3:%.*]] = select i1 [[T2]], i32 256, i32 0
 ; CHECK-NEXT:    call void @use1(i1 [[T2]])
 ; CHECK-NEXT:    ret i32 [[T3]]
 ;
@@ -438,8 +435,7 @@ define i32 @test15f_extra_use(i32 %X) {
 ; CHECK-LABEL: @test15f_extra_use(
 ; CHECK-NEXT:    [[T1:%.*]] = and i32 [[X:%.*]], 128
 ; CHECK-NEXT:    [[T2:%.*]] = icmp ne i32 [[T1]], 0
-; CHECK-NEXT:    [[TMP1:%.*]] = shl nuw nsw i32 [[T1]], 1
-; CHECK-NEXT:    [[T3:%.*]] = xor i32 [[TMP1]], 256
+; CHECK-NEXT:    [[T3:%.*]] = select i1 [[T2]], i32 0, i32 256
 ; CHECK-NEXT:    call void @use1(i1 [[T2]])
 ; CHECK-NEXT:    ret i32 [[T3]]
 ;
@@ -453,10 +449,9 @@ define i32 @test15f_extra_use(i32 %X) {
 ;; (a & 128) ? 0 : 256
 define i16 @test15f_trunc(i32 %X) {
 ; CHECK-LABEL: @test15f_trunc(
-; CHECK-NEXT:    [[TMP1:%.*]] = trunc i32 [[X:%.*]] to i16
-; CHECK-NEXT:    [[TMP2:%.*]] = shl i16 [[TMP1]], 1
-; CHECK-NEXT:    [[TMP3:%.*]] = and i16 [[TMP2]], 256
-; CHECK-NEXT:    [[T3:%.*]] = xor i16 [[TMP3]], 256
+; CHECK-NEXT:    [[T1:%.*]] = and i32 [[X:%.*]], 128
+; CHECK-NEXT:    [[T2_NOT:%.*]] = icmp eq i32 [[T1]], 0
+; CHECK-NEXT:    [[T3:%.*]] = select i1 [[T2_NOT]], i16 256, i16 0
 ; CHECK-NEXT:    ret i16 [[T3]]
 ;
   %t1 = and i32 %X, 128

dtcxzyw

LGTM. Thank you!

Can you pre-commit this test? https://alive2.llvm.org/ce/z/BWBK9_

define i32 @src(i32 %x) {
  %cmp = icmp sgt i32 %x, -1
  %ext = zext i1 %cmp to i32
  call void @use(i32 %ext)
  %and = and i32 %x, 2147483647
  %masksel = select i1 %cmp, i32 -2147483648, i32 0
  %res = or disjoint i32 %masksel, %and
  ret i32 %res
}

declare void @use(i32)

A possible solution: https://godbolt.org/z/TfdMKKb4E
convert

%and = and i32 %x, 2147483647
%masksel = select i1 %cmp, i32 -2147483648, i32 0
%res = or disjoint i32 %masksel, %and

into

  %and = and i32 %x, 2147483647 ; bset
  %or = or i32 %x, -2147483648 ; bclr
  %masksel = select i1 %.not9, i32 %or, i32 %and ; cmov

andjo403 · 2025-02-18T20:03:32Z

It is actually canonicalized the other way around see:

llvm-project/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

Lines 835 to 865 in 5ecce45

    
           /// Canonicalize a set or clear of a masked set of constant bits to 
        
           /// select-of-constants form. 
        
           static Instruction *foldSetClearBits(SelectInst &Sel, 
        
                                                InstCombiner::BuilderTy &Builder) { 
        
             Value *Cond = Sel.getCondition(); 
        
             Value *T = Sel.getTrueValue(); 
        
             Value *F = Sel.getFalseValue(); 
        
             Type *Ty = Sel.getType(); 
        
             Value *X; 
        
             const APInt *NotC, *C; 
        
             // Cond ? (X & ~C) : (X | C) --> (X & ~C) | (Cond ? 0 : C) 
        
             if (match(T, m_And(m_Value(X), m_APInt(NotC))) && 
        
                 match(F, m_OneUse(m_Or(m_Specific(X), m_APInt(C)))) && *NotC == ~(*C)) { 
        
               Constant *Zero = ConstantInt::getNullValue(Ty); 
        
               Constant *OrC = ConstantInt::get(Ty, *C); 
        
               Value *NewSel = Builder.CreateSelect(Cond, Zero, OrC, "masksel", &Sel); 
        
               return BinaryOperator::CreateOr(T, NewSel); 
        
             } 
        
             // Cond ? (X | C) : (X & ~C) --> (X & ~C) | (Cond ? C : 0) 
        
             if (match(F, m_And(m_Value(X), m_APInt(NotC))) && 
        
                 match(T, m_OneUse(m_Or(m_Specific(X), m_APInt(C)))) && *NotC == ~(*C)) { 
        
               Constant *Zero = ConstantInt::getNullValue(Ty); 
        
               Constant *OrC = ConstantInt::get(Ty, *C); 
        
               Value *NewSel = Builder.CreateSelect(Cond, OrC, Zero, "masksel", &Sel); 
        
               return BinaryOperator::CreateOr(F, NewSel); 
        
             } 
        
             return nullptr; 
        
           }

possible to add special case there for:
(icmp eq (X & C), 0) ? (X & ~C) : (X | C) -> X
(icmp eq (X & C), 0) ? (X | C) : (X & ~C) -> X ^ C
IFF C is a power of 2.

https://alive2.llvm.org/ce/z/dD3XTi

but is it common enough that special handling is wanted?

have added a test for the regression

dtcxzyw · 2025-02-19T12:22:59Z

It is actually canonicalized the other way around see:

llvm-project/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

Lines 835 to 865 in 5ecce45

/// Canonicalize a set or clear of a masked set of constant bits to

/// select-of-constants form.

static Instruction *foldSetClearBits(SelectInst &Sel,

InstCombiner::BuilderTy &Builder) {

Value *Cond = Sel.getCondition();

Value *T = Sel.getTrueValue();

Value *F = Sel.getFalseValue();

Type *Ty = Sel.getType();

Value *X;

const APInt *NotC, *C;

// Cond ? (X & ~C) : (X | C) --> (X & ~C) | (Cond ? 0 : C)

if (match(T, m_And(m_Value(X), m_APInt(NotC))) &&

match(F, m_OneUse(m_Or(m_Specific(X), m_APInt(C)))) && *NotC == ~(*C)) {

Constant *Zero = ConstantInt::getNullValue(Ty);

Constant *OrC = ConstantInt::get(Ty, *C);

Value *NewSel = Builder.CreateSelect(Cond, Zero, OrC, "masksel", &Sel);

return BinaryOperator::CreateOr(T, NewSel);

}

// Cond ? (X | C) : (X & ~C) --> (X & ~C) | (Cond ? C : 0)

if (match(F, m_And(m_Value(X), m_APInt(NotC))) &&

match(T, m_OneUse(m_Or(m_Specific(X), m_APInt(C)))) && *NotC == ~(*C)) {

Constant *Zero = ConstantInt::getNullValue(Ty);

Constant *OrC = ConstantInt::get(Ty, *C);

Value *NewSel = Builder.CreateSelect(Cond, OrC, Zero, "masksel", &Sel);

return BinaryOperator::CreateOr(F, NewSel);

}

return nullptr;

}

possible to add special case there for: (icmp eq (X & C), 0) ? (X & ~C) : (X | C) -> X (icmp eq (X & C), 0) ? (X | C) : (X & ~C) -> X ^ C IFF C is a power of 2.

https://alive2.llvm.org/ce/z/dD3XTi

but is it common enough that special handling is wanted?

have added a test for the regression

Original C code: https://godbolt.org/z/oMMao8dqa
Looks like this pattern is rare: https://github.com/dtcxzyw/llvm-tools/blob/main/pr127398.cpp

andjo403 requested review from nikic, dtcxzyw and goldsteinn February 16, 2025 14:32

llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Feb 16, 2025

This was referenced Feb 16, 2025

Task submission dtcxzyw/llvm-opt-benchmark#1312

Open

pre-commit: PR127398 dtcxzyw/llvm-opt-benchmark#2123

Closed

dtcxzyw approved these changes Feb 17, 2025

View reviewed changes

andjo403 added a commit that referenced this pull request Feb 18, 2025

[InstCombine] Add test for regression in #127398. (NFC)

ed14745

andjo403 force-pushed the avoidExtraInstructionsSelectIcmpAnd branch from 3741ad8 to 7495db0 Compare February 18, 2025 20:00

[InstCombine] avoid extra instructions in foldSelectICmpAnd

fe78e0f

andjo403 force-pushed the avoidExtraInstructionsSelectIcmpAnd branch from 7495db0 to fe78e0f Compare February 18, 2025 20:02

wldfngrs pushed a commit to wldfngrs/llvm-project that referenced this pull request Feb 19, 2025

[InstCombine] Add test for regression in llvm#127398. (NFC)

41dc11c

andjo403 merged commit 8fc03e4 into llvm:main Feb 19, 2025
8 checks passed

andjo403 deleted the avoidExtraInstructionsSelectIcmpAnd branch February 19, 2025 17:10

joaosaffran mentioned this pull request Feb 26, 2025

[libc++] Set feature-test macro __cpp_lib_atomic_float (#127559) joaosaffran/llvm-project#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[InstCombine] avoid extra instructions in foldSelectICmpAnd #127398

[InstCombine] avoid extra instructions in foldSelectICmpAnd #127398

Uh oh!

andjo403 commented Feb 16, 2025

Uh oh!

llvmbot commented Feb 16, 2025

Uh oh!

dtcxzyw left a comment

Uh oh!

andjo403 commented Feb 18, 2025 •

edited

Loading

Uh oh!

dtcxzyw commented Feb 19, 2025

Uh oh!

Uh oh!

Uh oh!

[InstCombine] avoid extra instructions in foldSelectICmpAnd #127398

[InstCombine] avoid extra instructions in foldSelectICmpAnd #127398

Uh oh!

Conversation

andjo403 commented Feb 16, 2025

Uh oh!

llvmbot commented Feb 16, 2025

Uh oh!

dtcxzyw left a comment

Choose a reason for hiding this comment

Uh oh!

andjo403 commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtcxzyw commented Feb 19, 2025

Uh oh!

Uh oh!

Uh oh!

andjo403 commented Feb 18, 2025 •

edited

Loading