[X86][SSE] Don't emit SSE2 load instructions in SSE1-only mode #134547

thrimbor · 2025-04-06T16:47:13Z

This fixes a regression I traced back to 8b43c1b / #79000

The regression caused an SSE2 instruction, movsd, to be emitted as a replacement for an SSE instruction, movaps despite the target potentially not supporting this instruction, such as when building with clang using -march=pentium3.

The test was produced by reducing down an actual occurrence of the issue in production code. I'm not super familiar with tests for optimization passes, so it may be possible to improve this further and I'll happily do so if advised.

The problematic optimization is part of the LLVM 19 and 20 releases, is it possible to have this fix backported and if yes, what's the process for that?

Fixes #134607

github-actions · 2025-04-06T16:47:30Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-04-06T16:48:06Z

@llvm/pr-subscribers-backend-x86

Author: Stefan Schmidt (thrimbor)

Changes

This fixes a regression I traced back to 8b43c1b / #79000

The regression caused an SSE2 instruction, movsd, to be emitted as a replacement for an SSE instruction, movaps despite the target potentially not supporting this instruction, such as when building with clang using -march=pentium3.

The test was produced by reducing down an actual occurrence of the issue in production code. I'm not super familiar with tests for optimization passes, so it may be possible to improve this further and I'll happily do so if advised.

The problematic optimization is part of the LLVM 19 and 20 releases, is it possible to have this fix backported and if yes, what's the process for that?

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

2 Files Affected:

(modified) llvm/lib/Target/X86/X86FixupVectorConstants.cpp (+5-3)
(modified) llvm/test/CodeGen/X86/sse1.ll (+13)

diff --git a/llvm/lib/Target/X86/X86FixupVectorConstants.cpp b/llvm/lib/Target/X86/X86FixupVectorConstants.cpp
index 40024baf93fdb..324167b53f5b6 100644
--- a/llvm/lib/Target/X86/X86FixupVectorConstants.cpp
+++ b/llvm/lib/Target/X86/X86FixupVectorConstants.cpp
@@ -333,6 +333,7 @@ bool X86FixupVectorConstantsPass::processInstruction(MachineFunction &MF,
                                                      MachineInstr &MI) {
   unsigned Opc = MI.getOpcode();
   MachineConstantPool *CP = MI.getParent()->getParent()->getConstantPool();
+  bool HasSSE2 = ST->hasSSE2();
   bool HasSSE41 = ST->hasSSE41();
   bool HasAVX2 = ST->hasAVX2();
   bool HasDQI = ST->hasDQI();
@@ -396,9 +397,10 @@ bool X86FixupVectorConstantsPass::processInstruction(MachineFunction &MF,
   case X86::MOVUPDrm:
   case X86::MOVUPSrm:
     // TODO: SSE3 MOVDDUP Handling
-    return FixupConstant({{X86::MOVSSrm, 1, 32, rebuildZeroUpperCst},
-                          {X86::MOVSDrm, 1, 64, rebuildZeroUpperCst}},
-                         128, 1);
+    return FixupConstant(
+        {{X86::MOVSSrm, 1, 32, rebuildZeroUpperCst},
+         {HasSSE2 ? X86::MOVSDrm : 0, 1, 64, rebuildZeroUpperCst}},
+        128, 1);
   case X86::VMOVAPDrm:
   case X86::VMOVAPSrm:
   case X86::VMOVUPDrm:
diff --git a/llvm/test/CodeGen/X86/sse1.ll b/llvm/test/CodeGen/X86/sse1.ll
index 8ac86d11d89e6..b5758c3356c82 100644
--- a/llvm/test/CodeGen/X86/sse1.ll
+++ b/llvm/test/CodeGen/X86/sse1.ll
@@ -251,5 +251,18 @@ define <2 x float> @PR31672() #0 {
 
 declare <2 x float> @llvm.sqrt.v2f32(<2 x float>) #1
 
+define void @movaps_test(ptr nocapture noundef writeonly %v) {
+; X86-LABEL: movaps_test:
+; X86:       # %bb.0:
+; X86-NEXT:    movl 4(%esp), %eax
+; X86-NEXT:    movaps {{\.?LCPI[0-9]+_[0-9]+}}, %xmm0
+
+; X64-LABEL: movaps_test:
+; X64:       # %bb.0:
+; X64-NEXT:    movaps {{\.?LCPI[0-9]+_[0-9]+}}(%rip), %xmm0
+  store <2 x float> <float 2.560000e+02, float 5.120000e+02>, ptr %v, align 4
+  ret void
+}
+
 attributes #0 = { nounwind "unsafe-fp-math"="true" }

RKSimon

Thanks for the fix - it should be a candidate for cherry picking back to 20.x once its been in trunk for a few days - we don't provide patches for 19.x any more though.

RKSimon · 2025-04-07T10:54:15Z

llvm/test/CodeGen/X86/sse1.ll

+; X64-NEXT:    movaps {{\.?LCPI[0-9]+_[0-9]+}}(%rip), %xmm0
+  store <2 x float> <float 2.560000e+02, float 5.120000e+02>, ptr %v, align 4
+  ret void
+}


This would be better adding to a new file - I've created #134607 to track this, so pr134607.ll would be a good name.

Take a look at the simplified repro I created in the issue (removes manyof the unnecessary function attributes).

RKSimon · 2025-04-07T10:57:25Z

llvm/lib/Target/X86/X86FixupVectorConstants.cpp

+    return FixupConstant(
+        {{X86::MOVSSrm, 1, 32, rebuildZeroUpperCst},
+         {HasSSE2 ? X86::MOVSDrm : 0, 1, 64, rebuildZeroUpperCst}},
+        128, 1);


Convert this to the FixupEntry Fixups[] = { pattern used below

RKSimon · 2025-04-08T09:38:26Z

llvm/test/CodeGen/X86/pr134607.ll

@@ -0,0 +1,15 @@
+; RUN: llc < %s -mtriple=i386-unknown-unknown -mattr=+sse -O3 | FileCheck %s --check-prefixes=X86
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=-sse2,+sse -O3 | FileCheck %s --check-prefixes=X64


Please can you add a SSE2 run as well to check we've not regressed:

RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=-sse2,+sse -O3 | FileCheck %s --check-prefixes=X64-SSE1 RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+sse2,+sse -O3 | FileCheck %s --check-prefixes=X64-SSE2

RKSimon

LGTM - cheers

github-actions · 2025-04-09T09:19:46Z

@thrimbor Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

…134547) This fixes a regression I traced back to llvm@8b43c1b / llvm#79000 The regression caused an SSE2 instruction, `movsd`, to be emitted as a replacement for an SSE instruction, `movaps` despite the target potentially not supporting this instruction, such as when building with clang using `-march=pentium3`. Fixes llvm#134607

…134547) This fixes a regression I traced back to llvm@8b43c1b / llvm#79000 The regression caused an SSE2 instruction, `movsd`, to be emitted as a replacement for an SSE instruction, `movaps` despite the target potentially not supporting this instruction, such as when building with clang using `-march=pentium3`. Fixes llvm#134607 (cherry picked from commit 08e080e)

…134547) This fixes a regression I traced back to llvm@8b43c1b / llvm#79000 The regression caused an SSE2 instruction, `movsd`, to be emitted as a replacement for an SSE instruction, `movaps` despite the target potentially not supporting this instruction, such as when building with clang using `-march=pentium3`. Fixes llvm#134607

llvmbot added the backend:X86 label Apr 6, 2025

RKSimon self-requested a review April 6, 2025 21:41

RKSimon mentioned this pull request Apr 7, 2025

[X86] X86FixupVectorConstants generates SSE2 instructions on SSE1 targets #134607

Closed

RKSimon requested changes Apr 7, 2025

View reviewed changes

RKSimon reviewed Apr 7, 2025

View reviewed changes

thrimbor force-pushed the sse_wrong_inst branch from 7e96421 to a492246 Compare April 8, 2025 03:03

RKSimon reviewed Apr 8, 2025

View reviewed changes

[X86][SSE] Don't emit SSE2 load instructions in SSE1-only mode

bd1d6b1

thrimbor force-pushed the sse_wrong_inst branch from a492246 to bd1d6b1 Compare April 8, 2025 15:31

RKSimon approved these changes Apr 8, 2025

View reviewed changes

RKSimon merged commit 08e080e into llvm:main Apr 9, 2025
11 checks passed

thrimbor deleted the sse_wrong_inst branch April 17, 2025 02:21

thrimbor mentioned this pull request Apr 17, 2025

build: Warn if clang version is in the 19.x.x-20.1.2 range XboxDev/nxdk#729

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[X86][SSE] Don't emit SSE2 load instructions in SSE1-only mode #134547

[X86][SSE] Don't emit SSE2 load instructions in SSE1-only mode #134547

thrimbor commented Apr 6, 2025 •

edited by RKSimon

Loading

github-actions bot commented Apr 6, 2025

llvmbot commented Apr 6, 2025

RKSimon left a comment

RKSimon Apr 7, 2025

thrimbor Apr 8, 2025

RKSimon Apr 7, 2025

thrimbor Apr 8, 2025

RKSimon Apr 8, 2025

thrimbor Apr 8, 2025

RKSimon left a comment

github-actions bot commented Apr 9, 2025

		@@ -0,0 +1,15 @@
		; RUN: llc < %s -mtriple=i386-unknown-unknown -mattr=+sse -O3 \| FileCheck %s --check-prefixes=X86
		; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=-sse2,+sse -O3 \| FileCheck %s --check-prefixes=X64

[X86][SSE] Don't emit SSE2 load instructions in SSE1-only mode #134547

[X86][SSE] Don't emit SSE2 load instructions in SSE1-only mode #134547

Conversation

thrimbor commented Apr 6, 2025 • edited by RKSimon Loading

github-actions bot commented Apr 6, 2025

llvmbot commented Apr 6, 2025

RKSimon left a comment

Choose a reason for hiding this comment

RKSimon Apr 7, 2025

Choose a reason for hiding this comment

thrimbor Apr 8, 2025

Choose a reason for hiding this comment

RKSimon Apr 7, 2025

Choose a reason for hiding this comment

thrimbor Apr 8, 2025

Choose a reason for hiding this comment

RKSimon Apr 8, 2025

Choose a reason for hiding this comment

thrimbor Apr 8, 2025

Choose a reason for hiding this comment

RKSimon left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 9, 2025

thrimbor commented Apr 6, 2025 •

edited by RKSimon

Loading