Skip to content

[InstCombine] Use m_NotForbidPoison when folding (X u< Y) ? -1 : (~X + Y) --> uadd.sat(~X, Y) #114345

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

Merged
merged 2 commits into from
Nov 1, 2024

Conversation

dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Oct 31, 2024

Alive2: https://alive2.llvm.org/ce/z/mTGCo-
We cannot reuse ~X if m_AllOnes matches a vector constant with some poison elts. An alternative solution is to create a new not instead of reusing ~X. But it doesn't worth the effort because we need to add a one-use check.

Fixes #113869.

@llvmbot
Copy link
Member

llvmbot commented Oct 31, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

Alive2: https://alive2.llvm.org/ce/z/mTGCo-
We cannot reuse ~X if m_AllOnes matches a vector constant with some poison elts. An alternative solution is to create a new not instead of reusing ~X. But it doesn't worth the effort because we need to add a one-use check.

Fixes #113869.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp (+1-1)
  • (modified) llvm/test/Transforms/InstCombine/saturating-add-sub.ll (+15)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index c5f39a4c381ed1..b442e79377740d 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -1051,7 +1051,7 @@ static Value *canonicalizeSaturatedAdd(ICmpInst *Cmp, Value *TVal, Value *FVal,
   // Strictness of the comparison is irrelevant.
   X = Cmp0;
   Y = Cmp1;
-  if (match(FVal, m_c_Add(m_Not(m_Specific(X)), m_Specific(Y)))) {
+  if (match(FVal, m_c_Add(m_NotForbidPoison(m_Specific(X)), m_Specific(Y)))) {
     // (X u< Y) ? -1 : (~X + Y) --> uadd.sat(~X, Y)
     // (X u< Y) ? -1 : (Y + ~X) --> uadd.sat(Y, ~X)
     BinaryOperator *BO = cast<BinaryOperator>(FVal);
diff --git a/llvm/test/Transforms/InstCombine/saturating-add-sub.ll b/llvm/test/Transforms/InstCombine/saturating-add-sub.ll
index a88fd3cc21f1bc..af8a9314a08049 100644
--- a/llvm/test/Transforms/InstCombine/saturating-add-sub.ll
+++ b/llvm/test/Transforms/InstCombine/saturating-add-sub.ll
@@ -1890,6 +1890,21 @@ define <2 x i32> @uadd_sat_not_ugt_commute_add(<2 x i32> %x, <2 x i32> %yp) {
   ret <2 x i32> %r
 }
 
+define <2 x i32> @uadd_sat_not_ugt_commute_add_partial_poison(<2 x i32> %x, <2 x i32> %yp) {
+; CHECK-LABEL: @uadd_sat_not_ugt_commute_add_partial_poison(
+; CHECK-NEXT:    [[NOTX:%.*]] = xor <2 x i32> [[X:%.*]], <i32 -1, i32 poison>
+; CHECK-NEXT:    [[A:%.*]] = add nuw <2 x i32> [[YP:%.*]], [[NOTX]]
+; CHECK-NEXT:    [[C:%.*]] = icmp ugt <2 x i32> [[YP]], [[X]]
+; CHECK-NEXT:    [[R:%.*]] = select <2 x i1> [[C]], <2 x i32> <i32 -1, i32 -1>, <2 x i32> [[A]]
+; CHECK-NEXT:    ret <2 x i32> [[R]]
+;
+  %notx = xor <2 x i32> %x, <i32 -1, i32 poison>
+  %a = add nuw <2 x i32> %yp, %notx
+  %c = icmp ugt <2 x i32> %yp, %x
+  %r = select <2 x i1> %c, <2 x i32> <i32 -1, i32 -1>, <2 x i32> %a
+  ret <2 x i32> %r
+}
+
 define i32 @uadd_sat_not_commute_select(i32 %x, i32 %y) {
 ; CHECK-LABEL: @uadd_sat_not_commute_select(
 ; CHECK-NEXT:    [[NOTX:%.*]] = xor i32 [[X:%.*]], -1

Copy link
Contributor

@goldsteinn goldsteinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM although I have trouble believing this is the only place we have this type of bug.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dtcxzyw dtcxzyw merged commit e577f14 into llvm:main Nov 1, 2024
10 checks passed
@dtcxzyw dtcxzyw deleted the fix-113869 branch November 1, 2024 14:18
@llvm-ci
Copy link
Collaborator

llvm-ci commented Nov 1, 2024

LLVM Buildbot has detected a new failure on builder lldb-remote-linux-ubuntu running on as-builder-9 while building llvm at step 16 "test-check-lldb-api".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/195/builds/478

Here is the relevant piece of the build log for the reference
Step 16 (test-check-lldb-api) failure: Test just built components: check-lldb-api completed (failure)
...
PASS: lldb-api :: types/TestIntegerType.py (1199 of 1208)
PASS: lldb-api :: types/TestIntegerTypeExpr.py (1200 of 1208)
PASS: lldb-api :: types/TestRecursiveTypes.py (1201 of 1208)
PASS: lldb-api :: types/TestShortType.py (1202 of 1208)
PASS: lldb-api :: types/TestShortTypeExpr.py (1203 of 1208)
PASS: lldb-api :: types/TestLongTypes.py (1204 of 1208)
PASS: lldb-api :: types/TestLongTypesExpr.py (1205 of 1208)
PASS: lldb-api :: tools/lldb-server/TestNonStop.py (1206 of 1208)
PASS: lldb-api :: tools/lldb-server/TestLldbGdbServer.py (1207 of 1208)
UNRESOLVED: lldb-api :: tools/lldb-server/TestGdbRemoteExpeditedRegisters.py (1208 of 1208)
******************** TEST 'lldb-api :: tools/lldb-server/TestGdbRemoteExpeditedRegisters.py' FAILED ********************
Script:
--
/usr/bin/python3.12 /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/dotest.py -u CXXFLAGS -u CFLAGS --env LLVM_LIBS_DIR=/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/./lib --env LLVM_INCLUDE_DIR=/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/include --env LLVM_TOOLS_DIR=/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/./bin --libcxx-include-dir /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/include/c++/v1 --libcxx-include-target-dir /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/include/aarch64-unknown-linux-gnu/c++/v1 --libcxx-library-dir /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/./lib/aarch64-unknown-linux-gnu --arch aarch64 --build-dir /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/lldb-test-build.noindex --lldb-module-cache-dir /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/./bin/lldb --compiler /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/bin/clang --dsymutil /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/./bin/dsymutil --make /usr/bin/make --llvm-tools-dir /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/./bin --lldb-obj-root /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/tools/lldb --lldb-libs-dir /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/./lib --platform-url connect://jetson-agx-2198.lab.llvm.org:1234 --platform-working-dir /home/ubuntu/lldb-tests --sysroot /mnt/fs/jetson-agx-ubuntu --env ARCH_CFLAGS=-mcpu=cortex-a78 --platform-name remote-linux /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/tools/lldb-server -p TestGdbRemoteExpeditedRegisters.py
--
Exit Code: 1

Command Output (stdout):
--
lldb version 20.0.0git (https://github.com/llvm/llvm-project.git revision e577f14b670ee2ae6bb717133310b215be4331b3)
  clang revision e577f14b670ee2ae6bb717133310b215be4331b3
  llvm revision e577f14b670ee2ae6bb717133310b215be4331b3
Setting up remote platform 'remote-linux'
Connecting to remote platform 'remote-linux' at 'connect://jetson-agx-2198.lab.llvm.org:1234'...
Connected.
Setting remote platform working directory to '/home/ubuntu/lldb-tests'...
Skipping the following test categories: ['dsym', 'gmodules', 'debugserver', 'objc', 'lldb-dap']
connect to debug monitor on port 13381 failed, attempt #1 of 20
connect to debug monitor on port 12607 failed, attempt #2 of 20
connect to debug monitor on port 13877 failed, attempt #3 of 20
connect to debug monitor on port 12917 failed, attempt #4 of 20
connect to debug monitor on port 13017 failed, attempt #5 of 20
connect to debug monitor on port 15489 failed, attempt #6 of 20
connect to debug monitor on port 12228 failed, attempt #7 of 20
connect to debug monitor on port 14472 failed, attempt #8 of 20
connect to debug monitor on port 14857 failed, attempt #9 of 20
connect to debug monitor on port 15571 failed, attempt #10 of 20
connect to debug monitor on port 15812 failed, attempt #11 of 20
connect to debug monitor on port 14429 failed, attempt #12 of 20
connect to debug monitor on port 13544 failed, attempt #13 of 20
connect to debug monitor on port 14323 failed, attempt #14 of 20
connect to debug monitor on port 13730 failed, attempt #15 of 20
connect to debug monitor on port 12537 failed, attempt #16 of 20
connect to debug monitor on port 12364 failed, attempt #17 of 20
connect to debug monitor on port 13132 failed, attempt #18 of 20
connect to debug monitor on port 15454 failed, attempt #19 of 20
connect to debug monitor on port 14659 failed, attempt #20 of 20

--

smallp-o-p pushed a commit to smallp-o-p/llvm-project that referenced this pull request Nov 3, 2024
…~X + Y) --> uadd.sat(~X, Y)` (llvm#114345)

Alive2: https://alive2.llvm.org/ce/z/mTGCo-
We cannot reuse `~X` if `m_AllOnes` matches a vector constant with some
poison elts. An alternative solution is to create a new not instead of
reusing `~X`. But it doesn't worth the effort because we need to add a
one-use check.

Fixes llvm#113869.
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…~X + Y) --> uadd.sat(~X, Y)` (llvm#114345)

Alive2: https://alive2.llvm.org/ce/z/mTGCo-
We cannot reuse `~X` if `m_AllOnes` matches a vector constant with some
poison elts. An alternative solution is to create a new not instead of
reusing `~X`. But it doesn't worth the effort because we need to add a
one-use check.

Fixes llvm#113869.
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.

Poison value wrongly propagates through vector operations on an optimized code
5 participants