[RISCV] Fix lowering of negative zero with Zdinx 32-bit #71869
Merged
Conversation
This file contains 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
The compiler currently abends with an impossible reg-to-reg copy when producing a negative zero FP immediate on RV32 with -Zdinx. This is because we emit a negation that uses FP registers. Emit the right node to produce correct code.
|
@llvm/pr-subscribers-backend-risc-v Author: Nemanja Ivanovic (nemanjai) ChangesThe compiler currently abends with an impossible reg-to-reg copy when producing a negative zero FP immediate on RV32 with -Zdinx. This is because we emit a negation that uses FP registers. Emit the right node to produce correct code. Full diff: https://github.com/llvm/llvm-project/pull/71869.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
index 1266c370cddeb5e..3f796f8d170e08a 100644
--- a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
@@ -901,6 +901,8 @@ void RISCVDAGToDAGISel::Select(SDNode *Node) {
Imm = selectImm(CurDAG, DL, XLenVT, APF.bitcastToAPInt().getSExtValue(),
*Subtarget);
+ bool HasZdinx = Subtarget->hasStdExtZdinx();
+ bool Is64Bit = Subtarget->is64Bit();
unsigned Opc;
switch (VT.SimpleTy) {
default:
@@ -920,8 +922,7 @@ void RISCVDAGToDAGISel::Select(SDNode *Node) {
// For RV32, we can't move from a GPR, we need to convert instead. This
// should only happen for +0.0 and -0.0.
assert((Subtarget->is64Bit() || APF.isZero()) && "Unexpected constant");
- bool HasZdinx = Subtarget->hasStdExtZdinx();
- if (Subtarget->is64Bit())
+ if (Is64Bit)
Opc = HasZdinx ? RISCV::COPY : RISCV::FMV_D_X;
else
Opc = HasZdinx ? RISCV::FCVT_D_W_IN32X : RISCV::FCVT_D_W;
@@ -937,9 +938,14 @@ void RISCVDAGToDAGISel::Select(SDNode *Node) {
Res = CurDAG->getMachineNode(Opc, DL, VT, Imm);
// For f64 -0.0, we need to insert a fneg.d idiom.
- if (NegZeroF64)
- Res = CurDAG->getMachineNode(RISCV::FSGNJN_D, DL, VT, SDValue(Res, 0),
+ if (NegZeroF64) {
+ if (Is64Bit)
+ Opc = HasZdinx ? RISCV::FSGNJN_D_INX : RISCV::FSGNJN_D;
+ else
+ Opc = HasZdinx ? RISCV::FSGNJN_D_IN32X : RISCV::FSGNJN_D;
+ Res = CurDAG->getMachineNode(Opc, DL, VT, SDValue(Res, 0),
SDValue(Res, 0));
+ }
ReplaceNode(Node, Res);
return;
diff --git a/llvm/test/CodeGen/RISCV/double-imm.ll b/llvm/test/CodeGen/RISCV/double-imm.ll
index dd554a8ce0dcf74..9254369baf19f3d 100644
--- a/llvm/test/CodeGen/RISCV/double-imm.ll
+++ b/llvm/test/CodeGen/RISCV/double-imm.ll
@@ -1,19 +1,25 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -mtriple=riscv32 -mattr=+d -verify-machineinstrs < %s \
-; RUN: -target-abi=ilp32d | FileCheck %s
+; RUN: -target-abi=ilp32d | FileCheck %s --check-prefix=CHECK32D
; RUN: llc -mtriple=riscv64 -mattr=+d -verify-machineinstrs < %s \
-; RUN: -target-abi=lp64d | FileCheck %s
+; RUN: -target-abi=lp64d | FileCheck %s --check-prefix=CHECK64D
; RUN: llc -mtriple=riscv32 -mattr=+zdinx -verify-machineinstrs < %s \
; RUN: -target-abi=ilp32 | FileCheck --check-prefix=CHECKRV32ZDINX %s
; RUN: llc -mtriple=riscv64 -mattr=+zdinx -verify-machineinstrs < %s \
; RUN: -target-abi=lp64 | FileCheck --check-prefix=CHECKRV64ZDINX %s
define double @double_imm() nounwind {
-; CHECK-LABEL: double_imm:
-; CHECK: # %bb.0:
-; CHECK-NEXT: lui a0, %hi(.LCPI0_0)
-; CHECK-NEXT: fld fa0, %lo(.LCPI0_0)(a0)
-; CHECK-NEXT: ret
+; CHECK32D-LABEL: double_imm:
+; CHECK32D: # %bb.0:
+; CHECK32D-NEXT: lui a0, %hi(.LCPI0_0)
+; CHECK32D-NEXT: fld fa0, %lo(.LCPI0_0)(a0)
+; CHECK32D-NEXT: ret
+;
+; CHECK64D-LABEL: double_imm:
+; CHECK64D: # %bb.0:
+; CHECK64D-NEXT: lui a0, %hi(.LCPI0_0)
+; CHECK64D-NEXT: fld fa0, %lo(.LCPI0_0)(a0)
+; CHECK64D-NEXT: ret
;
; CHECKRV32ZDINX-LABEL: double_imm:
; CHECKRV32ZDINX: # %bb.0:
@@ -32,12 +38,19 @@ define double @double_imm() nounwind {
}
define double @double_imm_op(double %a) nounwind {
-; CHECK-LABEL: double_imm_op:
-; CHECK: # %bb.0:
-; CHECK-NEXT: lui a0, %hi(.LCPI1_0)
-; CHECK-NEXT: fld fa5, %lo(.LCPI1_0)(a0)
-; CHECK-NEXT: fadd.d fa0, fa0, fa5
-; CHECK-NEXT: ret
+; CHECK32D-LABEL: double_imm_op:
+; CHECK32D: # %bb.0:
+; CHECK32D-NEXT: lui a0, %hi(.LCPI1_0)
+; CHECK32D-NEXT: fld fa5, %lo(.LCPI1_0)(a0)
+; CHECK32D-NEXT: fadd.d fa0, fa0, fa5
+; CHECK32D-NEXT: ret
+;
+; CHECK64D-LABEL: double_imm_op:
+; CHECK64D: # %bb.0:
+; CHECK64D-NEXT: lui a0, %hi(.LCPI1_0)
+; CHECK64D-NEXT: fld fa5, %lo(.LCPI1_0)(a0)
+; CHECK64D-NEXT: fadd.d fa0, fa0, fa5
+; CHECK64D-NEXT: ret
;
; CHECKRV32ZDINX-LABEL: double_imm_op:
; CHECKRV32ZDINX: # %bb.0:
@@ -68,6 +81,16 @@ define double @double_imm_op(double %a) nounwind {
}
define double @double_positive_zero(ptr %pd) nounwind {
+; CHECK32D-LABEL: double_positive_zero:
+; CHECK32D: # %bb.0:
+; CHECK32D-NEXT: fcvt.d.w fa0, zero
+; CHECK32D-NEXT: ret
+;
+; CHECK64D-LABEL: double_positive_zero:
+; CHECK64D: # %bb.0:
+; CHECK64D-NEXT: fmv.d.x fa0, zero
+; CHECK64D-NEXT: ret
+;
; CHECKRV32ZDINX-LABEL: double_positive_zero:
; CHECKRV32ZDINX: # %bb.0:
; CHECKRV32ZDINX-NEXT: li a0, 0
@@ -82,6 +105,18 @@ define double @double_positive_zero(ptr %pd) nounwind {
}
define double @double_negative_zero(ptr %pd) nounwind {
+; CHECK32D-LABEL: double_negative_zero:
+; CHECK32D: # %bb.0:
+; CHECK32D-NEXT: fcvt.d.w fa5, zero
+; CHECK32D-NEXT: fneg.d fa0, fa5
+; CHECK32D-NEXT: ret
+;
+; CHECK64D-LABEL: double_negative_zero:
+; CHECK64D: # %bb.0:
+; CHECK64D-NEXT: fmv.d.x fa5, zero
+; CHECK64D-NEXT: fneg.d fa0, fa5
+; CHECK64D-NEXT: ret
+;
; CHECKRV32ZDINX-LABEL: double_negative_zero:
; CHECKRV32ZDINX: # %bb.0:
; CHECKRV32ZDINX-NEXT: lui a1, 524288
@@ -95,3 +130,61 @@ define double @double_negative_zero(ptr %pd) nounwind {
; CHECKRV64ZDINX-NEXT: ret
ret double -0.0
}
+define dso_local double @negzero_sel(i16 noundef %a, double noundef %d) nounwind {
+; CHECK32D-LABEL: negzero_sel:
+; CHECK32D: # %bb.0: # %entry
+; CHECK32D-NEXT: slli a0, a0, 16
+; CHECK32D-NEXT: fcvt.d.w fa5, zero
+; CHECK32D-NEXT: beqz a0, .LBB4_2
+; CHECK32D-NEXT: # %bb.1: # %entry
+; CHECK32D-NEXT: fneg.d fa0, fa5
+; CHECK32D-NEXT: .LBB4_2: # %entry
+; CHECK32D-NEXT: ret
+;
+; CHECK64D-LABEL: negzero_sel:
+; CHECK64D: # %bb.0: # %entry
+; CHECK64D-NEXT: slli a0, a0, 48
+; CHECK64D-NEXT: beqz a0, .LBB4_2
+; CHECK64D-NEXT: # %bb.1: # %entry
+; CHECK64D-NEXT: fmv.d.x fa5, zero
+; CHECK64D-NEXT: fneg.d fa0, fa5
+; CHECK64D-NEXT: .LBB4_2: # %entry
+; CHECK64D-NEXT: ret
+;
+; CHECKRV32ZDINX-LABEL: negzero_sel:
+; CHECKRV32ZDINX: # %bb.0: # %entry
+; CHECKRV32ZDINX-NEXT: addi sp, sp, -16
+; CHECKRV32ZDINX-NEXT: sw a1, 8(sp)
+; CHECKRV32ZDINX-NEXT: sw a2, 12(sp)
+; CHECKRV32ZDINX-NEXT: slli a2, a0, 16
+; CHECKRV32ZDINX-NEXT: fcvt.d.w a0, zero
+; CHECKRV32ZDINX-NEXT: beqz a2, .LBB4_2
+; CHECKRV32ZDINX-NEXT: # %bb.1: # %entry
+; CHECKRV32ZDINX-NEXT: fneg.d a0, a0
+; CHECKRV32ZDINX-NEXT: j .LBB4_3
+; CHECKRV32ZDINX-NEXT: .LBB4_2:
+; CHECKRV32ZDINX-NEXT: lw a0, 8(sp)
+; CHECKRV32ZDINX-NEXT: lw a1, 12(sp)
+; CHECKRV32ZDINX-NEXT: .LBB4_3: # %entry
+; CHECKRV32ZDINX-NEXT: sw a0, 8(sp)
+; CHECKRV32ZDINX-NEXT: sw a1, 12(sp)
+; CHECKRV32ZDINX-NEXT: lw a0, 8(sp)
+; CHECKRV32ZDINX-NEXT: lw a1, 12(sp)
+; CHECKRV32ZDINX-NEXT: addi sp, sp, 16
+; CHECKRV32ZDINX-NEXT: ret
+;
+; CHECKRV64ZDINX-LABEL: negzero_sel:
+; CHECKRV64ZDINX: # %bb.0: # %entry
+; CHECKRV64ZDINX-NEXT: slli a2, a0, 48
+; CHECKRV64ZDINX-NEXT: beqz a2, .LBB4_2
+; CHECKRV64ZDINX-NEXT: # %bb.1: # %entry
+; CHECKRV64ZDINX-NEXT: fneg.d a0, zero
+; CHECKRV64ZDINX-NEXT: ret
+; CHECKRV64ZDINX-NEXT: .LBB4_2:
+; CHECKRV64ZDINX-NEXT: mv a0, a1
+; CHECKRV64ZDINX-NEXT: ret
+entry:
+ %tobool.not = icmp eq i16 %a, 0
+ %d. = select i1 %tobool.not, double %d, double -0.000000e+00
+ ret double %d.
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
topperc
reviewed
Nov 9, 2023
| if (NegZeroF64) | ||
| Res = CurDAG->getMachineNode(RISCV::FSGNJN_D, DL, VT, SDValue(Res, 0), | ||
| if (NegZeroF64) { | ||
| if (Is64Bit) |
There was a problem hiding this comment.
Maybe flip Is64Bit and HasZdinx to remove the duplicate case for !Zdinx.
if (HasZdinx)
Opc = Is64Bit ? RISCV::FSGNJN_D_INX : RISCV::FSGNJN_D_IN32X;
else
Opc = RISCV::FSGNJN_D
or
Opc = RISCV::FSGNJN_D;
if (HasZdinx)
Opc = Is64Bit ? RISCV::FSGNJN_D_INX : RISCV::FSGNJN_D_IN32X;
|
Glancing at the code post review, don't we have the same problem a couple of lines above for the FLI/ZFA codepath? Or is dinx and zfa an illegal combination in some way? And unrelated, but with dinx and zbs, couldn't we be better just using a bseti to materialize the sign that using the dedicated sign flip instruction? Not sure it hugely matters, but maybe? |
Zfa is defined to require F so it is not compatible with Zfinx/Zdinx. I think the thought was that several instructions in Zfa are redundant if you're using GPRs. |
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
The compiler currently abends with an impossible reg-to-reg copy when producing a negative zero FP immediate on RV32 with -Zdinx. This is because we emit a negation that uses FP registers. Emit the right node to produce correct code.
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.
The compiler currently abends with an impossible reg-to-reg copy when producing a negative zero FP immediate on RV32 with -Zdinx. This is because we emit a negation that uses FP registers. Emit the right node to produce correct code.