Skip to content

[RISCV] Fix incorrect codegen for Zfa with negated forms of constants in the lookup table #68026

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
Oct 2, 2023

Conversation

asb
Copy link
Contributor

@asb asb commented Oct 2, 2023

The logic in RISCVLoadFPImm::getLoadFPImm recognises that the only supported negative value is -1.0, but due to a typo returns false otherwise (entry 0, which is -1.0) rather than returning -1 (indicating no match found).

… in the lookup table

The logic in `RISCVLoadFPImm::getLoadFPImm` recognises that the only
supported negative value is -1.0, but due to a typo returns `false`
otherwise (entry 0, which is -1.0) rather than returning -1 (indicating
no match found).
@llvmbot
Copy link
Member

llvmbot commented Oct 2, 2023

@llvm/pr-subscribers-backend-risc-v

Changes

The logic in RISCVLoadFPImm::getLoadFPImm recognises that the only supported negative value is -1.0, but due to a typo returns false otherwise (entry 0, which is -1.0) rather than returning -1 (indicating no match found).


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

4 Files Affected:

  • (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp (+1-1)
  • (modified) llvm/test/CodeGen/RISCV/double-zfa.ll (+2-2)
  • (modified) llvm/test/CodeGen/RISCV/float-zfa.ll (+2-2)
  • (modified) llvm/test/CodeGen/RISCV/half-zfa-fli.ll (+2-2)
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
index 0a42c6faee29008..d71efc11e6a9fcf 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
@@ -273,7 +273,7 @@ int RISCVLoadFPImm::getLoadFPImm(APFloat FPImm) {
   if (Sign) {
     if (Entry == 16)
       return 0;
-    return false;
+    return -1;
   }
 
   return Entry;
diff --git a/llvm/test/CodeGen/RISCV/double-zfa.ll b/llvm/test/CodeGen/RISCV/double-zfa.ll
index 00481fb367136f0..d81b7ddafd81851 100644
--- a/llvm/test/CodeGen/RISCV/double-zfa.ll
+++ b/llvm/test/CodeGen/RISCV/double-zfa.ll
@@ -143,11 +143,11 @@ define double @loadfpimm16() {
 
 ; Ensure fli isn't incorrectly used for negated versions of numbers in the fli
 ; table.
-; FIXME: Codegen is incorrect.
 define double @loadfpimm17() {
 ; CHECK-LABEL: loadfpimm17:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fli.d fa0, -1.0
+; CHECK-NEXT:    lui a0, %hi(.LCPI15_0)
+; CHECK-NEXT:    fld fa0, %lo(.LCPI15_0)(a0)
 ; CHECK-NEXT:    ret
   ret double -2.0
 }
diff --git a/llvm/test/CodeGen/RISCV/float-zfa.ll b/llvm/test/CodeGen/RISCV/float-zfa.ll
index e41aa631d38a6ee..3b9043b0afe24b6 100644
--- a/llvm/test/CodeGen/RISCV/float-zfa.ll
+++ b/llvm/test/CodeGen/RISCV/float-zfa.ll
@@ -97,11 +97,11 @@ define float @loadfpimm11() {
 
 ; Ensure fli isn't incorrectly used for negated versions of numbers in the fli
 ; table.
-; FIXME: Codegen is incorrect.
 define float @loadfpimm12() {
 ; CHECK-LABEL: loadfpimm12:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fli.s fa0, -1.0
+; CHECK-NEXT:    lui a0, 786432
+; CHECK-NEXT:    fmv.w.x fa0, a0
 ; CHECK-NEXT:    ret
   ret float -2.0
 }
diff --git a/llvm/test/CodeGen/RISCV/half-zfa-fli.ll b/llvm/test/CodeGen/RISCV/half-zfa-fli.ll
index 90d5bb4b53ba46d..cab15e58b0bb823 100644
--- a/llvm/test/CodeGen/RISCV/half-zfa-fli.ll
+++ b/llvm/test/CodeGen/RISCV/half-zfa-fli.ll
@@ -197,11 +197,11 @@ define half @loadfpimm13() {
 
 ; Ensure fli isn't incorrectly used for negated versions of numbers in the fli
 ; table.
-; FIXME: Codegen is incorrect when Zfa is enabled.
 define half @loadfpimm14() {
 ; CHECK-LABEL: loadfpimm14:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fli.h fa0, -1.0
+; CHECK-NEXT:    lui a0, 1048572
+; CHECK-NEXT:    fmv.h.x fa0, a0
 ; CHECK-NEXT:    ret
 ;
 ; ZFHMIN-LABEL: loadfpimm14:

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

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

LGTM

@asb
Copy link
Contributor Author

asb commented Oct 2, 2023

It looks like this was the cause of all the test failures I saw with the GCC torture suite and Zfa (not that the torture suite exercises it particularly well - only ~50 tests see changed codegen). I'll do a quick test with a few other simple code bases, but after that I don't think there are blockers on following up on #67964 with a PR to mark Zfa as non-experimental.

I wasn't planning on suggesting this for 17.0.2 as I don't think we have a history or expectation of backporting fixes to experimental extensions (and it might send the wrong message about the degree of support we offer for them), but let me know if you feel it would be worth doing.

@asb asb merged commit 0152e1f into llvm:main Oct 2, 2023
asb added a commit to asb/llvm-project that referenced this pull request Oct 3, 2023
Following the version bump in llvm#67964 and the bug fix in llvm#68026 I believe
we're ready to mark Zfa as non-experimental. For what it's worth, the
GCC torture suite passes now (though it's more of a litmus test than
anything else).
asb added a commit that referenced this pull request Oct 3, 2023
Following the version bump in #67964 and the bug fix in #68026 I believe
we're ready to mark Zfa as non-experimental. I'll note the GCC torture
suite passes now with Zfa enabled (though it's more of a litmus test
than anything else).
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.

3 participants