[ConstantFold][RFC] Add AllowLHSConstant parameter in getBinOpAbsorber #109736

eric-xtang1008 · 2024-09-24T00:43:49Z

Add a AllowLHSConstant parameter in getBinOpAbsorber function for
supporting more binary operators.

llvmbot · 2024-09-24T00:44:24Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: None (eric-xtang1008)

Changes

Poison optimization in some corner case

Background

Let's start with one simple example e.c:

#include &lt;stdio.h&gt;
int main() {
    int b = 0;
    int c = -1;
    printf("0x%x\n", ((b &gt;&gt; c)) | 3);
    return 0;
}

Complied code and run:

$ clang e.c 
$ ./a.out
$ 0x3

If complied the code with "-O1" options, the result is random:

$ clang -O1 e.c
$ ./a.out
$ 0xe94fa2c8

The different result is that clang will optimize the result to poison, but hardware doesn't. Let's take a look of the LLVM IR code:

$ clang -emit-llvm -S e.c -o -
...
@.str = private unnamed_addr constant [6 x i8] c"0x%x\0A\00", align 1
; Function Attrs: noinline nounwind optnone uwtable
define dso_local i32 @<!-- -->main() #<!-- -->0 {
    %1 = alloca i32, align 4
    %2 = alloca i32, align 4
    %3 = alloca i32, align 4
    store i32 0, ptr %1, align 4
    store i32 0, ptr %2, align 4
    store i32 -1, ptr %3, align 4
    %4 = load i32, ptr %2, align 4
    %5 = load i32, ptr %3, align 4
    %6 = ashr i32 %4, %5
    %7 = or i32 %6, 3
    %8 = call i32 (ptr, ...) @<!-- -->printf(ptr noundef @.str, i32 noundef %7)
    ret i32 0
}
...

Another LLVM IR code (complied with "-O1"):

$ clang -O1 -emit-llvm -S e.c -o -
...
@.str = private unnamed_addr constant [6 x i8] c"0x%x\0A\00", align 1

; Function Attrs: nofree nounwind uwtable
define dso_local noundef i32 @<!-- -->main() local_unnamed_addr #<!-- -->0 {
    %1 = tail call i32 (ptr, ...) @<!-- -->printf(ptr noundef nonnull dereferenceable(1) @.str, i32 noundef poison)
    ret i32 0
}
...

Propose

There are several binary oprations that have the same behavor: if we known one oprand, we can infer the result direactly.

Or: -1 | X = -1
And: 0 &amp; X = 0
Mul: 0 * X = 0

Shl: 0 &lt;&lt; X = 0
Ashr: 0 &gt;&gt; a X = 0
Lshr: 0 &gt;&gt; l X = 0

I propose if the constant value C: C op X = C, fold it to C, for every X including undef and poison value.

If so, it will stop the propagation of undef and poison values, I don't known if it is appropriate, please help review it and comments.

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

7 Files Affected:

(modified) llvm/include/llvm/IR/Constants.h (+5-2)
(modified) llvm/lib/IR/ConstantFold.cpp (+14-21)
(modified) llvm/lib/IR/Constants.cpp (+19-6)
(modified) llvm/test/Transforms/InstCombine/vec_shuffle-inseltpoison.ll (+10-6)
(modified) llvm/test/Transforms/InstCombine/vec_shuffle.ll (+10-6)
(modified) llvm/test/Transforms/InstSimplify/undef.ll (+3-3)
(modified) llvm/test/Transforms/VectorCombine/X86/insert-binop-inseltpoison.ll (+1-1)

diff --git a/llvm/include/llvm/IR/Constants.h b/llvm/include/llvm/IR/Constants.h
index 3b16aa039a5087..18e92dcef448ce 100644
--- a/llvm/include/llvm/IR/Constants.h
+++ b/llvm/include/llvm/IR/Constants.h
@@ -1210,8 +1210,11 @@ class ConstantExpr : public Constant {
   /// Return the absorbing element for the given binary
   /// operation, i.e. a constant C such that X op C = C and C op X = C for
   /// every X.  For example, this returns zero for integer multiplication.
-  /// It returns null if the operator doesn't have an absorbing element.
-  static Constant *getBinOpAbsorber(unsigned Opcode, Type *Ty);
+  /// If AllowLHSConstant is true, operand 0 is a constant C that must be
+  /// defined as C op X = C. It returns null if the operator doesn't have
+  /// an absorbing element.
+  static Constant *getBinOpAbsorber(unsigned Opcode, Type *Ty,
+                                    bool AllowLHSConstant = false);
 
   /// Transparently provide more efficient getOperand methods.
   DECLARE_TRANSPARENT_OPERAND_ACCESSORS(Constant);
diff --git a/llvm/lib/IR/ConstantFold.cpp b/llvm/lib/IR/ConstantFold.cpp
index 05ab0968ef6f39..331decc51d4f29 100644
--- a/llvm/lib/IR/ConstantFold.cpp
+++ b/llvm/lib/IR/ConstantFold.cpp
@@ -624,6 +624,14 @@ Constant *llvm::ConstantFoldBinaryInstruction(unsigned Opcode, Constant *C1,
       return C1;
   }
 
+  if (Constant *Absorber = ConstantExpr::getBinOpAbsorber(
+          Opcode, C1->getType(), /*AllowLHSAbsorber*/ true)) {
+    if (C1 == Absorber)
+      return C1;
+    if (C2 == Absorber)
+      return C2;
+  }
+
   // Binary operations propagate poison.
   if (isa<PoisonValue>(C1) || isa<PoisonValue>(C2))
     return PoisonValue::get(C1->getType());
@@ -733,8 +741,7 @@ Constant *llvm::ConstantFoldBinaryInstruction(unsigned Opcode, Constant *C1,
   if (ConstantInt *CI2 = dyn_cast<ConstantInt>(C2)) {
     switch (Opcode) {
     case Instruction::Mul:
-      if (CI2->isZero())
-        return C2; // X * 0 == 0
+      assert(!CI2->isZero() && "Mul zero handled above");
       break;
     case Instruction::UDiv:
     case Instruction::SDiv:
@@ -749,9 +756,7 @@ Constant *llvm::ConstantFoldBinaryInstruction(unsigned Opcode, Constant *C1,
         return PoisonValue::get(CI2->getType());              // X % 0 == poison
       break;
     case Instruction::And:
-      if (CI2->isZero())
-        return C2; // X & 0 == 0
-
+      assert(!CI2->isZero() && "And zero handled above");
       if (ConstantExpr *CE1 = dyn_cast<ConstantExpr>(C1)) {
         // If and'ing the address of a global with a constant, fold it.
         if (CE1->getOpcode() == Instruction::PtrToInt &&
@@ -792,8 +797,7 @@ Constant *llvm::ConstantFoldBinaryInstruction(unsigned Opcode, Constant *C1,
       }
       break;
     case Instruction::Or:
-      if (CI2->isMinusOne())
-        return C2; // X | -1 == -1
+      assert(!CI2->isMinusOne() && "Or MinusOne handled above");
       break;
     }
   } else if (isa<ConstantInt>(C1)) {
@@ -840,33 +844,22 @@ Constant *llvm::ConstantFoldBinaryInstruction(unsigned Opcode, Constant *C1,
       case Instruction::Xor:
         return ConstantInt::get(CI1->getContext(), C1V ^ C2V);
       case Instruction::Shl:
+        assert(!CI1->isZero() && "Zero Shl handled above");
         if (C2V.ult(C1V.getBitWidth()))
           return ConstantInt::get(CI1->getContext(), C1V.shl(C2V));
         return PoisonValue::get(C1->getType()); // too big shift is poison
       case Instruction::LShr:
+        assert(!CI1->isZero() && "Zero LShr handled above");
         if (C2V.ult(C1V.getBitWidth()))
           return ConstantInt::get(CI1->getContext(), C1V.lshr(C2V));
         return PoisonValue::get(C1->getType()); // too big shift is poison
       case Instruction::AShr:
+        assert(!CI1->isZero() && "Zero AShr handled above");
         if (C2V.ult(C1V.getBitWidth()))
           return ConstantInt::get(CI1->getContext(), C1V.ashr(C2V));
         return PoisonValue::get(C1->getType()); // too big shift is poison
       }
     }
-
-    switch (Opcode) {
-    case Instruction::SDiv:
-    case Instruction::UDiv:
-    case Instruction::URem:
-    case Instruction::SRem:
-    case Instruction::LShr:
-    case Instruction::AShr:
-    case Instruction::Shl:
-      if (CI1->isZero()) return C1;
-      break;
-    default:
-      break;
-    }
   } else if (ConstantFP *CFP1 = dyn_cast<ConstantFP>(C1)) {
     if (ConstantFP *CFP2 = dyn_cast<ConstantFP>(C2)) {
       const APFloat &C1V = CFP1->getValueAPF();
diff --git a/llvm/lib/IR/Constants.cpp b/llvm/lib/IR/Constants.cpp
index d6c00a4b547829..bae6ef3ec81c2a 100644
--- a/llvm/lib/IR/Constants.cpp
+++ b/llvm/lib/IR/Constants.cpp
@@ -2735,17 +2735,30 @@ Constant *ConstantExpr::getIdentity(Instruction *I, Type *Ty,
   return nullptr;
 }
 
-Constant *ConstantExpr::getBinOpAbsorber(unsigned Opcode, Type *Ty) {
+Constant *ConstantExpr::getBinOpAbsorber(unsigned Opcode, Type *Ty,
+                                         bool AllowLHSConstant) {
   switch (Opcode) {
   default:
-    // Doesn't have an absorber.
-    return nullptr;
+    break;
 
-  case Instruction::Or:
+  case Instruction::Or:  // -1 | X = -1
     return Constant::getAllOnesValue(Ty);
 
-  case Instruction::And:
-  case Instruction::Mul:
+  case Instruction::And: // 0 & X = 0
+  case Instruction::Mul: // 0 * X = 0
+    return Constant::getNullValue(Ty);
+  }
+
+  // AllowLHSConstant must be set.
+  if (!AllowLHSConstant)
+    return nullptr;
+
+  switch (Opcode) {
+  default:
+    return nullptr;
+  case Instruction::Shl:  // 0 << X = 0
+  case Instruction::LShr: // 0 >>l X = 0
+  case Instruction::AShr: // 0 >>a X = 0
     return Constant::getNullValue(Ty);
   }
 }
diff --git a/llvm/test/Transforms/InstCombine/vec_shuffle-inseltpoison.ll b/llvm/test/Transforms/InstCombine/vec_shuffle-inseltpoison.ll
index 0f233fbb4729e6..c5f276ec908397 100644
--- a/llvm/test/Transforms/InstCombine/vec_shuffle-inseltpoison.ll
+++ b/llvm/test/Transforms/InstCombine/vec_shuffle-inseltpoison.ll
@@ -691,8 +691,8 @@ define <4 x i16> @widening_shuffle_shl_constant_op1_non0(<2 x i16> %v) {
 
 define <4 x i16> @widening_shuffle_or(<2 x i16> %v) {
 ; CHECK-LABEL: @widening_shuffle_or(
-; CHECK-NEXT:    [[TMP1:%.*]] = or <2 x i16> [[V:%.*]], <i16 42, i16 -42>
-; CHECK-NEXT:    [[BO:%.*]] = shufflevector <2 x i16> [[TMP1]], <2 x i16> poison, <4 x i32> <i32 0, i32 1, i32 poison, i32 poison>
+; CHECK-NEXT:    [[SHUF:%.*]] = shufflevector <2 x i16> [[V:%.*]], <2 x i16> poison, <4 x i32> <i32 0, i32 1, i32 poison, i32 poison>
+; CHECK-NEXT:    [[BO:%.*]] = or <4 x i16> [[SHUF]], <i16 42, i16 -42, i16 -1, i16 -1>
 ; CHECK-NEXT:    ret <4 x i16> [[BO]]
 ;
   %shuf = shufflevector <2 x i16> %v, <2 x i16> poison, <4 x i32> <i32 0, i32 1, i32 undef, i32 undef>
@@ -1035,7 +1035,8 @@ define <2 x i32> @and_splat_constant(<2 x i32> %x) {
 define <4 x i16> @and_constant_mask_poison(<4 x i16> %add) {
 ; CHECK-LABEL: @and_constant_mask_poison(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[AND:%.*]] = shufflevector <4 x i16> [[ADD:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 poison, i32 1, i32 1>
+; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <4 x i16> [[ADD:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 poison, i32 1, i32 1>
+; CHECK-NEXT:    [[AND:%.*]] = and <4 x i16> [[SHUFFLE]], <i16 0, i16 0, i16 -1, i16 -1>
 ; CHECK-NEXT:    ret <4 x i16> [[AND]]
 ;
 entry:
@@ -1047,7 +1048,8 @@ entry:
 define <4 x i16> @and_constant_mask_poison_2(<4 x i16> %add) {
 ; CHECK-LABEL: @and_constant_mask_poison_2(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[AND:%.*]] = shufflevector <4 x i16> [[ADD:%.*]], <4 x i16> poison, <4 x i32> <i32 1, i32 1, i32 1, i32 poison>
+; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <4 x i16> [[ADD:%.*]], <4 x i16> poison, <4 x i32> <i32 1, i32 1, i32 1, i32 poison>
+; CHECK-NEXT:    [[AND:%.*]] = and <4 x i16> [[SHUFFLE]], <i16 -1, i16 -1, i16 -1, i16 0>
 ; CHECK-NEXT:    ret <4 x i16> [[AND]]
 ;
 entry:
@@ -1098,7 +1100,8 @@ entry:
 define <4 x i16> @or_constant_mask_poison(<4 x i16> %in) {
 ; CHECK-LABEL: @or_constant_mask_poison(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[OR:%.*]] = shufflevector <4 x i16> [[IN:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 poison, i32 1, i32 1>
+; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <4 x i16> [[IN:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 poison, i32 1, i32 1>
+; CHECK-NEXT:    [[OR:%.*]] = or <4 x i16> [[SHUFFLE]], <i16 -1, i16 -1, i16 0, i16 0>
 ; CHECK-NEXT:    ret <4 x i16> [[OR]]
 ;
 entry:
@@ -1110,7 +1113,8 @@ entry:
 define <4 x i16> @or_constant_mask_poison_2(<4 x i16> %in) {
 ; CHECK-LABEL: @or_constant_mask_poison_2(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[OR:%.*]] = shufflevector <4 x i16> [[IN:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 1, i32 1, i32 poison>
+; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <4 x i16> [[IN:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 1, i32 1, i32 poison>
+; CHECK-NEXT:    [[OR:%.*]] = or <4 x i16> [[SHUFFLE]], <i16 -1, i16 0, i16 0, i16 -1>
 ; CHECK-NEXT:    ret <4 x i16> [[OR]]
 ;
 entry:
diff --git a/llvm/test/Transforms/InstCombine/vec_shuffle.ll b/llvm/test/Transforms/InstCombine/vec_shuffle.ll
index 75a84e51279b80..aaa46a0dfcd151 100644
--- a/llvm/test/Transforms/InstCombine/vec_shuffle.ll
+++ b/llvm/test/Transforms/InstCombine/vec_shuffle.ll
@@ -685,8 +685,8 @@ define <4 x i16> @widening_shuffle_shl_constant_op1_non0(<2 x i16> %v) {
 
 define <4 x i16> @widening_shuffle_or(<2 x i16> %v) {
 ; CHECK-LABEL: @widening_shuffle_or(
-; CHECK-NEXT:    [[TMP1:%.*]] = or <2 x i16> [[V:%.*]], <i16 42, i16 -42>
-; CHECK-NEXT:    [[BO:%.*]] = shufflevector <2 x i16> [[TMP1]], <2 x i16> poison, <4 x i32> <i32 0, i32 1, i32 poison, i32 poison>
+; CHECK-NEXT:    [[SHUF:%.*]] = shufflevector <2 x i16> [[V:%.*]], <2 x i16> poison, <4 x i32> <i32 0, i32 1, i32 poison, i32 poison>
+; CHECK-NEXT:    [[BO:%.*]] = or <4 x i16> [[SHUF]], <i16 42, i16 -42, i16 -1, i16 -1>
 ; CHECK-NEXT:    ret <4 x i16> [[BO]]
 ;
   %shuf = shufflevector <2 x i16> %v, <2 x i16> undef, <4 x i32> <i32 0, i32 1, i32 undef, i32 undef>
@@ -1029,7 +1029,8 @@ define <2 x i32> @and_splat_constant(<2 x i32> %x) {
 define <4 x i16> @and_constant_mask_poison(<4 x i16> %add) {
 ; CHECK-LABEL: @and_constant_mask_poison(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[AND:%.*]] = shufflevector <4 x i16> [[ADD:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 poison, i32 1, i32 1>
+; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <4 x i16> [[ADD:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 poison, i32 1, i32 1>
+; CHECK-NEXT:    [[AND:%.*]] = and <4 x i16> [[SHUFFLE]], <i16 0, i16 0, i16 -1, i16 -1>
 ; CHECK-NEXT:    ret <4 x i16> [[AND]]
 ;
 entry:
@@ -1041,7 +1042,8 @@ entry:
 define <4 x i16> @and_constant_mask_poison_2(<4 x i16> %add) {
 ; CHECK-LABEL: @and_constant_mask_poison_2(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[AND:%.*]] = shufflevector <4 x i16> [[ADD:%.*]], <4 x i16> poison, <4 x i32> <i32 1, i32 1, i32 1, i32 poison>
+; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <4 x i16> [[ADD:%.*]], <4 x i16> poison, <4 x i32> <i32 1, i32 1, i32 1, i32 poison>
+; CHECK-NEXT:    [[AND:%.*]] = and <4 x i16> [[SHUFFLE]], <i16 -1, i16 -1, i16 -1, i16 0>
 ; CHECK-NEXT:    ret <4 x i16> [[AND]]
 ;
 entry:
@@ -1092,7 +1094,8 @@ entry:
 define <4 x i16> @or_constant_mask_poison(<4 x i16> %in) {
 ; CHECK-LABEL: @or_constant_mask_poison(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[OR:%.*]] = shufflevector <4 x i16> [[IN:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 poison, i32 1, i32 1>
+; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <4 x i16> [[IN:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 poison, i32 1, i32 1>
+; CHECK-NEXT:    [[OR:%.*]] = or <4 x i16> [[SHUFFLE]], <i16 -1, i16 -1, i16 0, i16 0>
 ; CHECK-NEXT:    ret <4 x i16> [[OR]]
 ;
 entry:
@@ -1104,7 +1107,8 @@ entry:
 define <4 x i16> @or_constant_mask_poison_2(<4 x i16> %in) {
 ; CHECK-LABEL: @or_constant_mask_poison_2(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[OR:%.*]] = shufflevector <4 x i16> [[IN:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 1, i32 1, i32 poison>
+; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <4 x i16> [[IN:%.*]], <4 x i16> poison, <4 x i32> <i32 poison, i32 1, i32 1, i32 poison>
+; CHECK-NEXT:    [[OR:%.*]] = or <4 x i16> [[SHUFFLE]], <i16 -1, i16 0, i16 0, i16 -1>
 ; CHECK-NEXT:    ret <4 x i16> [[OR]]
 ;
 entry:
diff --git a/llvm/test/Transforms/InstSimplify/undef.ll b/llvm/test/Transforms/InstSimplify/undef.ll
index b32744fe8a3bb1..fee4372b31a22f 100644
--- a/llvm/test/Transforms/InstSimplify/undef.ll
+++ b/llvm/test/Transforms/InstSimplify/undef.ll
@@ -244,7 +244,7 @@ define i32 @test24() {
 
 define i32 @test25() {
 ; CHECK-LABEL: @test25(
-; CHECK-NEXT:    ret i32 poison
+; CHECK-NEXT:    ret i32 0
 ;
   %b = lshr i32 0, undef
   ret i32 %b
@@ -252,7 +252,7 @@ define i32 @test25() {
 
 define i32 @test26() {
 ; CHECK-LABEL: @test26(
-; CHECK-NEXT:    ret i32 poison
+; CHECK-NEXT:    ret i32 0
 ;
   %b = ashr i32 0, undef
   ret i32 %b
@@ -260,7 +260,7 @@ define i32 @test26() {
 
 define i32 @test27() {
 ; CHECK-LABEL: @test27(
-; CHECK-NEXT:    ret i32 poison
+; CHECK-NEXT:    ret i32 0
 ;
   %b = shl i32 0, undef
   ret i32 %b
diff --git a/llvm/test/Transforms/VectorCombine/X86/insert-binop-inseltpoison.ll b/llvm/test/Transforms/VectorCombine/X86/insert-binop-inseltpoison.ll
index c1100780254c18..d3863a274a9dd4 100644
--- a/llvm/test/Transforms/VectorCombine/X86/insert-binop-inseltpoison.ll
+++ b/llvm/test/Transforms/VectorCombine/X86/insert-binop-inseltpoison.ll
@@ -100,7 +100,7 @@ define <16 x i8> @ins1_ins0_add(i8 %x, i8 %y) {
 define <4 x i32> @ins0_ins0_mul(i32 %x, i32 %y) {
 ; CHECK-LABEL: @ins0_ins0_mul(
 ; CHECK-NEXT:    [[R_SCALAR:%.*]] = mul i32 [[X:%.*]], [[Y:%.*]]
-; CHECK-NEXT:    [[R:%.*]] = insertelement <4 x i32> poison, i32 [[R_SCALAR]], i64 0
+; CHECK-NEXT:    [[R:%.*]] = insertelement <4 x i32> zeroinitializer, i32 [[R_SCALAR]], i64 0
 ; CHECK-NEXT:    ret <4 x i32> [[R]]
 ;
   %i0 = insertelement <4 x i32> zeroinitializer, i32 %x, i32 0

github-actions · 2024-09-24T00:47:13Z

✅ With the latest revision this PR passed the C/C++ code formatter.

nikic

The different result is that clang will optimize the result to poison, but hardware doesn't.

What the hardware does here is not relevant, only the semantics specified in LangRef are relevant. Doing what you're doing here may look like it "fixes" the problem, but as long as your input contains undefined behavior, there will always be more cases where is "misbehaves".

Adding LHS support to getBinOpAbsorber() by itself is a reasonable idea, but it would have to be implemented in a way that preserves the current behavior for poison.

eric-xtang1008 · 2024-09-24T10:04:43Z

Thanks.

The different result is that clang will optimize the result to poison, but hardware doesn't.

What the hardware does here is not relevant, only the semantics specified in LangRef are relevant.

Yes, that's good point. I misunderstood before.

Doing what you're doing here may look like it "fixes" the problem, but as long as your input contains undefined behavior, there will always be more cases where is "misbehaves".

In my understand, the input may be constant, undef and posion value in this condition, so how to understand undefined behavior?

Adding LHS support to getBinOpAbsorber() by itself is a reasonable idea, but it would have to be implemented in a way that preserves the current behavior for poison.

Yes, this patch changed the behavior of poison, but only for special value(0 & -1) in some special binary ops(Or, And, Mul, Shfit), like as 0 * poison = 0. In my mind, 0 multily any integer number equal 0, but before this patch, 0 * poison = poison, so how to understand 0 * poison = poison?

For shift, this change (0 << -1 = 0) could be right? (before it's (0 << -1 = poison)).

nikic · 2024-09-24T10:14:56Z

Please see https://llvm.org/docs/LangRef.html#poisonvalues for the semantics of poison values. In most cases, it's "poison in, poison out". So 0 * poison is indeed poison. (This is not the case for undef, where 0 * undef is indeed 0 and not undef.)

eric-xtang1008 · 2024-09-26T09:54:29Z

Thanks a lot, @nikic!

"What the hardware does here is not relevant, only the semantics specified in LangRef are relevant." There is another explain here. The code above caused an undefined behavior if we ananlyse it based on C standard(the same as optimizer do), we shouldn't write the code like above.

I update the patch only refactor the getBinOpAbsober function, please help review it again if you have time, thanks！

llvm/include/llvm/IR/Constants.h

nikic · 2024-09-26T13:06:24Z

llvm/lib/IR/ConstantFold.cpp

@@ -729,13 +729,15 @@ Constant *llvm::ConstantFoldBinaryInstruction(unsigned Opcode, Constant *C1,
  // Neither constant should be UndefValue, unless these are vector constants.
  assert((!HasScalarUndefOrScalableVectorUndef) && "Unexpected UndefValue");

+  Constant *Absorber = ConstantExpr::getBinOpAbsorber(
+      Opcode, C1->getType(), /*AllowLHSAbsorber*/ true);


Suggested change

Opcode, C1->getType(), /*AllowLHSAbsorber*/ true);

Opcode, C1->getType(), /*AllowLHSConstant*/ true);

To match the parameter name.

nikic · 2024-09-26T13:09:33Z

llvm/lib/IR/ConstantFold.cpp

  // Handle simplifications when the RHS is a constant int.
  if (ConstantInt *CI2 = dyn_cast<ConstantInt>(C2)) {
+    if (C2 == Absorber)
+      return C2;


This looks incorrect. You are using the Absorber value with AllowLHSAbsorber=true and then using it to check the RHS. This means that for example X << 0 could get incorrectly folded to 0, no? I think the only reason this works out here is by accident, because this case is already handled by the identity logic earlier.

Yes, I modified it. Please help review it, thanks!

nikic

LGTM

preames · 2024-09-27T15:22:25Z

LGTM

When landing, please make sure to remember to update the cover message. The actual commit message is fine, but the cover message is actively misleading and should not be used for the commit message (as per github's default.)

…r function Add a AllowLHSConstant parameter in getBinOpAbsorber function for supporting more Binary operators. Signed-off-by: eric.tang <[email protected]>

eric-xtang1008 · 2024-09-29T12:17:09Z

LGTM

When landing, please make sure to remember to update the cover message. The actual commit message is fine, but the cover message is actively misleading and should not be used for the commit message (as per github's default.)

I changed the cover message to "Add AllowLHSConstant parameter in getBinOpAbsorber function", does it make sense?

llvm#109736) Add a AllowLHSConstant parameter in getBinOpAbsorber function for supporting more binary operators.

llvmbot added llvm:ir llvm:transforms labels Sep 24, 2024

eric-xtang1008 force-pushed the dev-poison-opt branch from fd2da3b to 87dd67f Compare September 24, 2024 00:54

eric-xtang1008 requested review from nikic and preames September 24, 2024 07:12

nikic requested changes Sep 24, 2024

View reviewed changes

eric-xtang1008 force-pushed the dev-poison-opt branch from 87dd67f to 4d33224 Compare September 26, 2024 08:50

eric-xtang1008 changed the title ~~[ConstantFold][RFC] Fold special constant value with binop absorber f…~~ [ConstantFold][RFC] Refactor getBinOpAbsorber function Sep 26, 2024

nikic reviewed Sep 26, 2024

View reviewed changes

eric-xtang1008 force-pushed the dev-poison-opt branch 2 times, most recently from 2fcc008 to f93fac8 Compare September 27, 2024 06:52

nikic approved these changes Sep 27, 2024

View reviewed changes

eric-xtang1008 changed the title ~~[ConstantFold][RFC] Refactor getBinOpAbsorber function~~ [ConstantFold][RFC] Add AllowLHSConstant parameter in getBinOpAbsorber Sep 29, 2024

[ConstantFold][RFC] Add AllowLHSConstant parameter in getBinOpAbsorbe…

a34285b

…r function Add a AllowLHSConstant parameter in getBinOpAbsorber function for supporting more Binary operators. Signed-off-by: eric.tang <[email protected]>

eric-xtang1008 force-pushed the dev-poison-opt branch from f93fac8 to a34285b Compare September 29, 2024 12:09

nikic merged commit a59e5d8 into llvm:main Oct 1, 2024
8 checks passed

eric-xtang1008 deleted the dev-poison-opt branch October 9, 2024 00:29

	Opcode, C1->getType(), /AllowLHSAbsorber/ true);
	Opcode, C1->getType(), /AllowLHSConstant/ true);

[ConstantFold][RFC] Add AllowLHSConstant parameter in getBinOpAbsorber #109736

[ConstantFold][RFC] Add AllowLHSConstant parameter in getBinOpAbsorber #109736

Uh oh!

Conversation

eric-xtang1008 commented Sep 24, 2024 • edited by nikic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Poison optimization in some corner case

Background

Propose

Uh oh!

github-actions bot commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

eric-xtang1008 commented Sep 24, 2024

Uh oh!

nikic commented Sep 24, 2024

Uh oh!

eric-xtang1008 commented Sep 26, 2024

Uh oh!

Uh oh!

nikic Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

eric-xtang1008 Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

nikic Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

eric-xtang1008 Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

preames commented Sep 27, 2024

Uh oh!

eric-xtang1008 commented Sep 29, 2024

Uh oh!

Uh oh!

Uh oh!

eric-xtang1008 commented Sep 24, 2024 •

edited by nikic

Loading

llvmbot commented Sep 24, 2024 •

edited

Loading

github-actions bot commented Sep 24, 2024 •

edited

Loading