[FPEnv][X86] Correct strictfp tests. #87791
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Correct strictfp tests to follow the rules documented in the LangRef: https://llvm.org/docs/LangRef.html#constrained-floating-point-intrinsics These tests needed the strictfp attribute added to some function definitions. FP wait instructions now appear as a result. Test changes verified with D146845.
|
@llvm/pr-subscribers-backend-x86 Author: Kevin P. Neal (kpneal) ChangesCorrect strictfp tests to follow the rules documented in the LangRef: These tests needed the strictfp attribute added to some function Test changes verified with D146845. I'm not sure the new wait instructions count as "obvious", and I wanted an X86 expert to look at them anyway to be sure. Full diff: https://github.com/llvm/llvm-project/pull/87791.diff 2 Files Affected:
diff --git a/llvm/test/CodeGen/X86/strict-fadd-combines.ll b/llvm/test/CodeGen/X86/strict-fadd-combines.ll
index e0c61ac8d395e3..14944fab7d00dc 100644
--- a/llvm/test/CodeGen/X86/strict-fadd-combines.ll
+++ b/llvm/test/CodeGen/X86/strict-fadd-combines.ll
@@ -2,7 +2,7 @@
; RUN: llc < %s -mtriple=i686-unknown-unknown -mattr=+sse2 | FileCheck %s --check-prefixes=X86
; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+sse2 | FileCheck %s --check-prefixes=X64
-define float @fneg_strict_fadd_to_strict_fsub(float %x, float %y) nounwind {
+define float @fneg_strict_fadd_to_strict_fsub(float %x, float %y) nounwind strictfp {
; X86-LABEL: fneg_strict_fadd_to_strict_fsub:
; X86: # %bb.0:
; X86-NEXT: pushl %eax
@@ -10,6 +10,7 @@ define float @fneg_strict_fadd_to_strict_fsub(float %x, float %y) nounwind {
; X86-NEXT: subss {{[0-9]+}}(%esp), %xmm0
; X86-NEXT: movss %xmm0, (%esp)
; X86-NEXT: flds (%esp)
+; X86-NEXT: wait
; X86-NEXT: popl %eax
; X86-NEXT: retl
;
@@ -22,7 +23,7 @@ define float @fneg_strict_fadd_to_strict_fsub(float %x, float %y) nounwind {
ret float %add
}
-define float @fneg_strict_fadd_to_strict_fsub_2(float %x, float %y) nounwind {
+define float @fneg_strict_fadd_to_strict_fsub_2(float %x, float %y) nounwind strictfp {
; X86-LABEL: fneg_strict_fadd_to_strict_fsub_2:
; X86: # %bb.0:
; X86-NEXT: pushl %eax
@@ -30,6 +31,7 @@ define float @fneg_strict_fadd_to_strict_fsub_2(float %x, float %y) nounwind {
; X86-NEXT: subss {{[0-9]+}}(%esp), %xmm0
; X86-NEXT: movss %xmm0, (%esp)
; X86-NEXT: flds (%esp)
+; X86-NEXT: wait
; X86-NEXT: popl %eax
; X86-NEXT: retl
;
@@ -42,7 +44,7 @@ define float @fneg_strict_fadd_to_strict_fsub_2(float %x, float %y) nounwind {
ret float %add
}
-define double @fneg_strict_fadd_to_strict_fsub_d(double %x, double %y) nounwind {
+define double @fneg_strict_fadd_to_strict_fsub_d(double %x, double %y) nounwind strictfp {
; X86-LABEL: fneg_strict_fadd_to_strict_fsub_d:
; X86: # %bb.0:
; X86-NEXT: pushl %ebp
@@ -53,6 +55,7 @@ define double @fneg_strict_fadd_to_strict_fsub_d(double %x, double %y) nounwind
; X86-NEXT: subsd 16(%ebp), %xmm0
; X86-NEXT: movsd %xmm0, (%esp)
; X86-NEXT: fldl (%esp)
+; X86-NEXT: wait
; X86-NEXT: movl %ebp, %esp
; X86-NEXT: popl %ebp
; X86-NEXT: retl
@@ -66,7 +69,7 @@ define double @fneg_strict_fadd_to_strict_fsub_d(double %x, double %y) nounwind
ret double %add
}
-define double @fneg_strict_fadd_to_strict_fsub_2d(double %x, double %y) nounwind {
+define double @fneg_strict_fadd_to_strict_fsub_2d(double %x, double %y) nounwind strictfp {
; X86-LABEL: fneg_strict_fadd_to_strict_fsub_2d:
; X86: # %bb.0:
; X86-NEXT: pushl %ebp
@@ -77,6 +80,7 @@ define double @fneg_strict_fadd_to_strict_fsub_2d(double %x, double %y) nounwind
; X86-NEXT: subsd 16(%ebp), %xmm0
; X86-NEXT: movsd %xmm0, (%esp)
; X86-NEXT: fldl (%esp)
+; X86-NEXT: wait
; X86-NEXT: movl %ebp, %esp
; X86-NEXT: popl %ebp
; X86-NEXT: retl
diff --git a/llvm/test/CodeGen/X86/strict-fsub-combines.ll b/llvm/test/CodeGen/X86/strict-fsub-combines.ll
index 8cb591a980e11e..774ea02ccd87a4 100644
--- a/llvm/test/CodeGen/X86/strict-fsub-combines.ll
+++ b/llvm/test/CodeGen/X86/strict-fsub-combines.ll
@@ -3,7 +3,7 @@
; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+sse2 | FileCheck %s --check-prefixes=X64
; FIXME: Missing fsub(x,fneg(y)) -> fadd(x,y) fold
-define float @fneg_strict_fsub_to_strict_fadd(float %x, float %y) nounwind {
+define float @fneg_strict_fsub_to_strict_fadd(float %x, float %y) nounwind strictfp {
; X86-LABEL: fneg_strict_fsub_to_strict_fadd:
; X86: # %bb.0:
; X86-NEXT: pushl %eax
@@ -13,6 +13,7 @@ define float @fneg_strict_fsub_to_strict_fadd(float %x, float %y) nounwind {
; X86-NEXT: subss %xmm1, %xmm0
; X86-NEXT: movss %xmm0, (%esp)
; X86-NEXT: flds (%esp)
+; X86-NEXT: wait
; X86-NEXT: popl %eax
; X86-NEXT: retl
;
@@ -27,7 +28,7 @@ define float @fneg_strict_fsub_to_strict_fadd(float %x, float %y) nounwind {
}
; FIXME: Missing fsub(x,fneg(y)) -> fadd(x,y) fold
-define double @fneg_strict_fsub_to_strict_fadd_d(double %x, double %y) nounwind {
+define double @fneg_strict_fsub_to_strict_fadd_d(double %x, double %y) nounwind strictfp {
; X86-LABEL: fneg_strict_fsub_to_strict_fadd_d:
; X86: # %bb.0:
; X86-NEXT: pushl %ebp
@@ -40,6 +41,7 @@ define double @fneg_strict_fsub_to_strict_fadd_d(double %x, double %y) nounwind
; X86-NEXT: subsd %xmm1, %xmm0
; X86-NEXT: movsd %xmm0, (%esp)
; X86-NEXT: fldl (%esp)
+; X86-NEXT: wait
; X86-NEXT: movl %ebp, %esp
; X86-NEXT: popl %ebp
; X86-NEXT: retl
@@ -55,7 +57,7 @@ define double @fneg_strict_fsub_to_strict_fadd_d(double %x, double %y) nounwind
}
; FIXME: Missing fneg(fsub(x,y)) -> fsub(y,x) fold
-define float @strict_fsub_fneg_to_strict_fsub(float %x, float %y) nounwind {
+define float @strict_fsub_fneg_to_strict_fsub(float %x, float %y) nounwind strictfp {
; X86-LABEL: strict_fsub_fneg_to_strict_fsub:
; X86: # %bb.0:
; X86-NEXT: pushl %eax
@@ -64,6 +66,7 @@ define float @strict_fsub_fneg_to_strict_fsub(float %x, float %y) nounwind {
; X86-NEXT: xorps {{\.?LCPI[0-9]+_[0-9]+}}, %xmm0
; X86-NEXT: movss %xmm0, (%esp)
; X86-NEXT: flds (%esp)
+; X86-NEXT: wait
; X86-NEXT: popl %eax
; X86-NEXT: retl
;
@@ -78,7 +81,7 @@ define float @strict_fsub_fneg_to_strict_fsub(float %x, float %y) nounwind {
}
; FIXME: Missing fneg(fsub(x,y)) -> fsub(y,x) fold
-define double @strict_fsub_fneg_to_strict_fsub_d(double %x, double %y) nounwind {
+define double @strict_fsub_fneg_to_strict_fsub_d(double %x, double %y) nounwind strictfp {
; X86-LABEL: strict_fsub_fneg_to_strict_fsub_d:
; X86: # %bb.0:
; X86-NEXT: pushl %ebp
@@ -90,6 +93,7 @@ define double @strict_fsub_fneg_to_strict_fsub_d(double %x, double %y) nounwind
; X86-NEXT: xorpd {{\.?LCPI[0-9]+_[0-9]+}}, %xmm0
; X86-NEXT: movlpd %xmm0, (%esp)
; X86-NEXT: fldl (%esp)
+; X86-NEXT: wait
; X86-NEXT: movl %ebp, %esp
; X86-NEXT: popl %ebp
; X86-NEXT: retl
|
andykaylor
approved these changes
Apr 5, 2024
There was a problem hiding this comment.
This looks right to me.
You may know this, but I'll comment here to have the explanation with your patch. The x87 exception handling is a bit arcane. When an exception occurs, there are few guarantees about when it will be reported, but a wait instruction acts as a barrier to ensure that nothing else happens until any pending exceptions are processed. I believe the C language standard allows a bit of leeway about the timing of exceptions, but it requires them to be handled before making a function call or returning from a function. It seems that's the model that LLVM is following.
FWIW, the Intel Classic Compiler (icc) inserts waits all over the place, more or less after every x87 instruction, when strict exception semantics are enabled. That seems like overkill as you'd pretty much need to block all optimizations to maintain that level of granularity in the exception handling.
It's a bit disturbing that LLVM doesn't insert the wait instructions without the strictfp attribute, but I suppose once you've finished the work to enforce the rules for this that will be ok.
|
Thank you! I wonder if this is another argument for not relying on math attributes attached to the function definition? |
kpneal
added a commit
that referenced
this pull request
Apr 8, 2024
Correct a strictfp test to follow the rules documented in the LangRef: https://llvm.org/docs/LangRef.html#constrained-floating-point-intrinsics This test needed the strictfp attribute added to some function definitions. FP wait instructions now appear as a result. The need for the wait instructions is explained by Andy Kaylor in PR#87791: #87791 Test changes verified with D146845.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Correct strictfp tests to follow the rules documented in the LangRef:
https://llvm.org/docs/LangRef.html#constrained-floating-point-intrinsics
These tests needed the strictfp attribute added to some function
definitions. FP wait instructions now appear as a result.
Test changes verified with D146845.
I'm not sure the new wait instructions count as "obvious", and I wanted an X86 expert to look at them anyway to be sure.