Skip to content

[GlobalISel] Clear nsw flags when converting sub to add. #137288

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 1 commit into from
Apr 26, 2025

Conversation

davemgreen
Copy link
Collaborator

As shown in https://alive2.llvm.org/ce/z/PVwcTL we need to clear the nsw flags too when converting a sub to a add if the constant is INT_MIN.

Fixes #137254

As shown in https://alive2.llvm.org/ce/z/PVwcTL we need to clear the nsw flags
too when converting a sub to a add if the constant is INT_MIN.

Fixes llvm#137254
@llvmbot
Copy link
Member

llvmbot commented Apr 25, 2025

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-aarch64

Author: David Green (davemgreen)

Changes

As shown in https://alive2.llvm.org/ce/z/PVwcTL we need to clear the nsw flags too when converting a sub to a add if the constant is INT_MIN.

Fixes #137254


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

3 Files Affected:

  • (modified) llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp (+2)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-trivial-arith.mir (+45)
  • (modified) llvm/test/CodeGen/AArch64/sub1.ll (+14)
diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index 333f0c17bacc5..5191360c7718a 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -2112,6 +2112,8 @@ bool CombinerHelper::matchCombineSubToAdd(MachineInstr &MI,
     MI.setDesc(B.getTII().get(TargetOpcode::G_ADD));
     MI.getOperand(2).setReg(NegCst.getReg(0));
     MI.clearFlag(MachineInstr::MIFlag::NoUWrap);
+    if (Imm.isMinSignedValue())
+      MI.clearFlags(MachineInstr::MIFlag::NoSWrap);
     Observer.changedInstr(MI);
   };
   return true;
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-trivial-arith.mir b/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-trivial-arith.mir
index 4c3faa9403909..de95527ceb1a2 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-trivial-arith.mir
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-trivial-arith.mir
@@ -551,3 +551,48 @@ body:             |
     RET_ReallyLR implicit $w0
 
 ...
+---
+name:   sub_to_add_nsw_128
+body:             |
+  bb.0:
+    liveins: $w0, $w1
+
+    ; CHECK-LABEL: name: sub_to_add_nsw_128
+    ; CHECK: liveins: $w0, $w1
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: %a:_(s32) = COPY $w0
+    ; CHECK-NEXT: %b:_(s8) = G_TRUNC %a(s32)
+    ; CHECK-NEXT: %c:_(s8) = G_CONSTANT i8 -128
+    ; CHECK-NEXT: %add:_(s8) = G_ADD %b, %c
+    ; CHECK-NEXT: %d:_(s32) = G_ZEXT %add(s8)
+    ; CHECK-NEXT: $w0 = COPY %d(s32)
+    ; CHECK-NEXT: RET_ReallyLR implicit $x0
+    %a:_(s32) = COPY $w0
+    %b:_(s8) = G_TRUNC %a
+    %c:_(s8) = G_CONSTANT i8 -128
+    %add:_(s8) = nsw nuw G_SUB %b, %c
+    %d:_(s32) = G_ZEXT %add
+    $w0 = COPY %d
+    RET_ReallyLR implicit $x0
+
+...
+---
+name:   sub_to_add_nsw_intmin
+body:             |
+  bb.0:
+    liveins: $w0, $w1
+
+    ; CHECK-LABEL: name: sub_to_add_nsw_intmin
+    ; CHECK: liveins: $w0, $w1
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: %a:_(s32) = COPY $w0
+    ; CHECK-NEXT: %c:_(s32) = G_CONSTANT i32 -2147483648
+    ; CHECK-NEXT: %add:_(s32) = G_ADD %a, %c
+    ; CHECK-NEXT: $w0 = COPY %add(s32)
+    ; CHECK-NEXT: RET_ReallyLR implicit $x0
+    %a:_(s32) = COPY $w0
+    %c:_(s32) = G_CONSTANT i32 -2147483648
+    %add:_(s32) = nsw nuw G_SUB %a, %c
+    $w0 = COPY %add
+    RET_ReallyLR implicit $x0
+...
diff --git a/llvm/test/CodeGen/AArch64/sub1.ll b/llvm/test/CodeGen/AArch64/sub1.ll
index 02893e2b63287..c359be57c27da 100644
--- a/llvm/test/CodeGen/AArch64/sub1.ll
+++ b/llvm/test/CodeGen/AArch64/sub1.ll
@@ -117,3 +117,17 @@ define <4 x i32> @masked_sub_v4i32(<4 x i32> %x) {
   %m = sub <4 x i32> <i32 511, i32 511, i32 511, i32 511>, %a
   ret <4 x i32>  %m
 }
+
+define i32 @pr137254(i32 %0) {
+; CHECK-LABEL: pr137254:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    mov w8, #-2147483648 // =0x80000000
+; CHECK-NEXT:    add w8, w0, w8
+; CHECK-NEXT:    cmp w8, #0
+; CHECK-NEXT:    cset w0, gt
+; CHECK-NEXT:    ret
+  %2 = sub nsw i32 %0, -2147483648
+  %3 = icmp sgt i32 %2, 0
+  %4 = select i1 %3, i32 1, i32 0
+  ret i32 %4
+}

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

@davemgreen davemgreen merged commit b9e3274 into llvm:main Apr 26, 2025
14 checks passed
@davemgreen davemgreen deleted the gh-gi-subnswminint branch April 26, 2025 10:01
@llvm-ci
Copy link
Collaborator

llvm-ci commented Apr 26, 2025

LLVM Buildbot has detected a new failure on builder lldb-aarch64-ubuntu running on linaro-lldb-aarch64-ubuntu while building llvm at step 6 "test".

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

Here is the relevant piece of the build log for the reference
Step 6 (test) failure: build (failure)
...
PASS: lldb-api :: tools/lldb-server/TestGdbRemoteAttach.py (1209 of 2151)
UNSUPPORTED: lldb-api :: tools/lldb-server/TestGdbRemoteForkResume.py (1210 of 2151)
PASS: lldb-api :: tools/lldb-server/TestGdbRemoteCompletion.py (1211 of 2151)
PASS: lldb-api :: tools/lldb-server/TestGdbRemoteExitCode.py (1212 of 2151)
PASS: lldb-api :: tools/lldb-server/TestGdbRemoteHostInfo.py (1213 of 2151)
PASS: lldb-api :: tools/lldb-server/TestGdbRemoteModuleInfo.py (1214 of 2151)
PASS: lldb-api :: tools/lldb-server/TestGdbRemoteAuxvSupport.py (1215 of 2151)
PASS: lldb-api :: terminal/TestEditlineCompletions.py (1216 of 2151)
UNSUPPORTED: lldb-api :: tools/lldb-server/TestGdbRemoteSaveCore.py (1217 of 2151)
UNRESOLVED: lldb-api :: tools/lldb-dap/variables/TestDAP_variables.py (1218 of 2151)
******************** TEST 'lldb-api :: tools/lldb-dap/variables/TestDAP_variables.py' FAILED ********************
Script:
--
/usr/bin/python3.10 /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/dotest.py -u CXXFLAGS -u CFLAGS --env LLVM_LIBS_DIR=/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./lib --env LLVM_INCLUDE_DIR=/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/include --env LLVM_TOOLS_DIR=/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin --arch aarch64 --build-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex --lldb-module-cache-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin/lldb --compiler /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin/clang --dsymutil /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin/dsymutil --make /usr/bin/gmake --llvm-tools-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin --lldb-obj-root /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb --lldb-libs-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./lib /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/tools/lldb-dap/variables -p TestDAP_variables.py
--
Exit Code: 1

Command Output (stdout):
--
lldb version 21.0.0git (https://github.com/llvm/llvm-project.git revision b9e32749d273a957e60170d6e7ef205fd1fb1834)
  clang revision b9e32749d273a957e60170d6e7ef205fd1fb1834
  llvm revision b9e32749d273a957e60170d6e7ef205fd1fb1834
Skipping the following test categories: ['libc++', 'dsym', 'gmodules', 'debugserver', 'objc']

--
Command Output (stderr):
--
UNSUPPORTED: LLDB (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/clang-aarch64) :: test_darwin_dwarf_missing_obj (TestDAP_variables.TestDAP_variables) (requires one of macosx, darwin, ios, tvos, watchos, bridgeos, iphonesimulator, watchsimulator, appletvsimulator) 
UNSUPPORTED: LLDB (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/clang-aarch64) :: test_darwin_dwarf_missing_obj_with_symbol_ondemand_enabled (TestDAP_variables.TestDAP_variables) (requires one of macosx, darwin, ios, tvos, watchos, bridgeos, iphonesimulator, watchsimulator, appletvsimulator) 
========= DEBUG ADAPTER PROTOCOL LOGS =========
1745662231.796308279 --> (stdin/stdout) {"command":"initialize","type":"request","arguments":{"adapterID":"lldb-native","clientID":"vscode","columnsStartAt1":true,"linesStartAt1":true,"locale":"en-us","pathFormat":"path","supportsRunInTerminalRequest":true,"supportsVariablePaging":true,"supportsVariableType":true,"supportsStartDebuggingRequest":true,"supportsProgressReporting":true,"$__lldb_sourceInitFile":false},"seq":1}
1745662231.798393965 <-- (stdin/stdout) {"body":{"$__lldb_version":"lldb version 21.0.0git (https://github.com/llvm/llvm-project.git revision b9e32749d273a957e60170d6e7ef205fd1fb1834)\n  clang revision b9e32749d273a957e60170d6e7ef205fd1fb1834\n  llvm revision b9e32749d273a957e60170d6e7ef205fd1fb1834","completionTriggerCharacters":["."," ","\t"],"exceptionBreakpointFilters":[{"default":false,"filter":"cpp_catch","label":"C++ Catch"},{"default":false,"filter":"cpp_throw","label":"C++ Throw"},{"default":false,"filter":"objc_catch","label":"Objective-C Catch"},{"default":false,"filter":"objc_throw","label":"Objective-C Throw"}],"supportTerminateDebuggee":true,"supportsBreakpointLocationsRequest":true,"supportsCancelRequest":true,"supportsCompletionsRequest":true,"supportsConditionalBreakpoints":true,"supportsConfigurationDoneRequest":true,"supportsDataBreakpoints":true,"supportsDelayedStackTraceLoading":true,"supportsDisassembleRequest":true,"supportsEvaluateForHovers":true,"supportsExceptionInfoRequest":true,"supportsExceptionOptions":true,"supportsFunctionBreakpoints":true,"supportsHitConditionalBreakpoints":true,"supportsInstructionBreakpoints":true,"supportsLogPoints":true,"supportsModulesRequest":true,"supportsReadMemoryRequest":true,"supportsRestartRequest":true,"supportsSetVariable":true,"supportsStepInTargetsRequest":true,"supportsSteppingGranularity":true,"supportsValueFormattingOptions":true},"command":"initialize","request_seq":1,"seq":0,"success":true,"type":"response"}
1745662231.798626423 --> (stdin/stdout) {"command":"launch","type":"request","arguments":{"program":"/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/tools/lldb-dap/variables/TestDAP_variables.test_indexedVariables/a.out","initCommands":["settings clear -all","settings set symbols.enable-external-lookup false","settings set target.inherit-tcc true","settings set target.disable-aslr false","settings set target.detach-on-error false","settings set target.auto-apply-fixits false","settings set plugin.process.gdb-remote.packet-timeout 60","settings set symbols.clang-modules-cache-path \"/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/module-cache-lldb/lldb-api\"","settings set use-color false","settings set show-statusline false"],"disableASLR":false,"enableAutoVariableSummaries":false,"enableSyntheticChildDebugging":false,"displayExtendedBacktrace":false},"seq":2}
1745662231.798836946 <-- (stdin/stdout) {"body":{"category":"console","output":"Running initCommands:\n"},"event":"output","seq":0,"type":"event"}
1745662231.798863173 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings clear -all\n"},"event":"output","seq":0,"type":"event"}
1745662231.798874378 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set symbols.enable-external-lookup false\n"},"event":"output","seq":0,"type":"event"}
1745662231.798884630 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set target.inherit-tcc true\n"},"event":"output","seq":0,"type":"event"}
1745662231.798894644 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set target.disable-aslr false\n"},"event":"output","seq":0,"type":"event"}
1745662231.798902988 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set target.detach-on-error false\n"},"event":"output","seq":0,"type":"event"}
1745662231.798911095 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set target.auto-apply-fixits false\n"},"event":"output","seq":0,"type":"event"}
1745662231.798918724 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set plugin.process.gdb-remote.packet-timeout 60\n"},"event":"output","seq":0,"type":"event"}
1745662231.798941612 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set symbols.clang-modules-cache-path \"/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/module-cache-lldb/lldb-api\"\n"},"event":"output","seq":0,"type":"event"}
1745662231.798951626 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set use-color false\n"},"event":"output","seq":0,"type":"event"}
1745662231.798959732 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set show-statusline false\n"},"event":"output","seq":0,"type":"event"}
1745662231.875898361 <-- (stdin/stdout) {"command":"launch","request_seq":2,"seq":0,"success":true,"type":"response"}
1745662231.875951529 <-- (stdin/stdout) {"body":{"isLocalProcess":true,"name":"/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/tools/lldb-dap/variables/TestDAP_variables.test_indexedVariables/a.out","startMethod":"launch","systemProcessId":2844046},"event":"process","seq":0,"type":"event"}
1745662231.875962973 <-- (stdin/stdout) {"event":"initialized","seq":0,"type":"event"}
1745662231.876236916 --> (stdin/stdout) {"command":"setBreakpoints","type":"request","arguments":{"source":{"name":"main.cpp","path":"main.cpp"},"sourceModified":false,"lines":[40],"breakpoints":[{"line":40}]},"seq":3}
1745662231.877703190 <-- (stdin/stdout) {"body":{"breakpoints":[{"column":1,"id":1,"instructionReference":"0xAAAAAED30C54","line":41,"source":{"name":"main.cpp","path":"main.cpp"},"verified":true}]},"command":"setBreakpoints","request_seq":3,"seq":0,"success":true,"type":"response"}

jyli0116 pushed a commit to jyli0116/llvm-project that referenced this pull request Apr 28, 2025
As shown in https://alive2.llvm.org/ce/z/PVwcTL we need to clear the nsw
flags too when converting a sub to a add if the constant is INT_MIN.

Fixes llvm#137254
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
As shown in https://alive2.llvm.org/ce/z/PVwcTL we need to clear the nsw
flags too when converting a sub to a add if the constant is INT_MIN.

Fixes llvm#137254
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
As shown in https://alive2.llvm.org/ce/z/PVwcTL we need to clear the nsw
flags too when converting a sub to a add if the constant is INT_MIN.

Fixes llvm#137254
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
As shown in https://alive2.llvm.org/ce/z/PVwcTL we need to clear the nsw
flags too when converting a sub to a add if the constant is INT_MIN.

Fixes llvm#137254
Ankur-0429 pushed a commit to Ankur-0429/llvm-project that referenced this pull request May 9, 2025
As shown in https://alive2.llvm.org/ce/z/PVwcTL we need to clear the nsw
flags too when converting a sub to a add if the constant is INT_MIN.

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

integer math miscompile from AArch64 global isel backend
4 participants