[RISCV] Use FLI + FNEG to materialize some negative FP constants

[mshockwave]

Most of the FP constants supported by FLI are positive. For negative FP constants X whose positive values is supported by FLI, we can use (FNEG (FLI -X)) to materialize X.

[llvmbot]

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

Author: Min-Yih Hsu (mshockwave)

Changes

Most of the FP constants supported by FLI are positive. For negative FP constants X whose positive values is supported by FLI, we can use (FNEG (FLI -X)) to materialize X.

---

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

6 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp (+17-5)
• (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+18-7)
• (modified) llvm/lib/Target/RISCV/RISCVISelLowering.h (+1-1)
• (modified) llvm/test/CodeGen/RISCV/double-zfa.ll (+6-6)
• (modified) llvm/test/CodeGen/RISCV/float-zfa.ll (+6-6)
• (modified) llvm/test/CodeGen/RISCV/half-zfa-fli.ll (+6-7)

diff --git a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp index c2cac993fe13c4b..9f3c387914944b7 100644 --- a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp +++ b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp @@ -857,26 +857,34 @@ void RISCVDAGToDAGISel::Select(SDNode *Node) { } case ISD::ConstantFP: { const APFloat &APF = cast<ConstantFPSDNode>(Node)->getValueAPF(); - int FPImm = static_cast<const RISCVTargetLowering *>(TLI)->getLegalZfaFPImm( - APF, VT); + auto [FPImm, NeedsFNeg] = + static_cast<const RISCVTargetLowering *>(TLI)->getLegalZfaFPImm(APF, + VT); if (FPImm >= 0) { unsigned Opc; + unsigned FNegOpc; switch (VT.SimpleTy) { default: llvm_unreachable("Unexpected size"); case MVT::f16: Opc = RISCV::FLI_H; + FNegOpc = RISCV::FSGNJN_H; break; case MVT::f32: Opc = RISCV::FLI_S; + FNegOpc = RISCV::FSGNJN_S; break; case MVT::f64: Opc = RISCV::FLI_D; + FNegOpc = RISCV::FSGNJN_D; break; } - SDNode *Res = CurDAG->getMachineNode( Opc, DL, VT, CurDAG->getTargetConstant(FPImm, DL, XLenVT)); + if (NeedsFNeg) + Res = CurDAG->getMachineNode(FNegOpc, DL, VT, SDValue(Res, 0), + SDValue(Res, 0)); + ReplaceNode(Node, Res); return; } @@ -3200,8 +3208,12 @@ bool RISCVDAGToDAGISel::selectFPImm(SDValue N, SDValue &Imm) { MVT VT = CFP->getSimpleValueType(0); - if (static_cast<const RISCVTargetLowering *>(TLI)->getLegalZfaFPImm(APF, - VT) >= 0) + // Even if this FPImm requires an additional FNEG (i.e. the second element of + // the returned pair is true) we still prefer FLI + FNEG over immediate + // materialization as the latter might generate a longer instruction sequence. + if (static_cast<const RISCVTargetLowering *>(TLI) + ->getLegalZfaFPImm(APF, VT) + .first >= 0) return false; MVT XLenVT = Subtarget->getXLenVT(); diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp index beb371063f89b2d..e9f80432ab190c7 100644 --- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp +++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp @@ -1978,11 +1978,17 @@ bool RISCVTargetLowering::isOffsetFoldingLegal( return false; } -// Returns 0-31 if the fli instruction is available for the type and this is -// legal FP immediate for the type. Returns -1 otherwise. -int RISCVTargetLowering::getLegalZfaFPImm(const APFloat &Imm, EVT VT) const { +// Return one of the followings: +// (1) `{0-31 value, false}` if FLI is available for Imm's type and FP value. +// (2) `{0-31 value, true}` if Imm is negative and FLI is available for its +// positive counterpart, which will be materialized from the first returned +// element. The second returned element indicated that there should be a FNEG +// followed. +// (3) `{-1, _}` if there is no way FLI can be used to materialize Imm. +std::pair<int, bool> RISCVTargetLowering::getLegalZfaFPImm(const APFloat &Imm, + EVT VT) const { if (!Subtarget.hasStdExtZfa()) - return -1; + return std::make_pair(-1, false); bool IsSupportedVT = false; if (VT == MVT::f16) { @@ -1995,9 +2001,14 @@ int RISCVTargetLowering::getLegalZfaFPImm(const APFloat &Imm, EVT VT) const { } if (!IsSupportedVT) - return -1; + return std::make_pair(-1, false); - return RISCVLoadFPImm::getLoadFPImm(Imm); + int Index = RISCVLoadFPImm::getLoadFPImm(Imm); + if (Index < 0 && Imm.isNegative()) + // Try the combination of its positive counterpart + FNEG. + return std::make_pair(RISCVLoadFPImm::getLoadFPImm(-Imm), true); + else + return std::make_pair(Index, false); } bool RISCVTargetLowering::isFPImmLegal(const APFloat &Imm, EVT VT, @@ -2015,7 +2026,7 @@ bool RISCVTargetLowering::isFPImmLegal(const APFloat &Imm, EVT VT, if (!IsLegalVT) return false; - if (getLegalZfaFPImm(Imm, VT) >= 0) + if (getLegalZfaFPImm(Imm, VT).first >= 0) return true; // Cannot create a 64 bit floating-point immediate value for rv32. diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.h b/llvm/lib/Target/RISCV/RISCVISelLowering.h index 5ca6376f858c44d..49dd01eccb02a0b 100644 --- a/llvm/lib/Target/RISCV/RISCVISelLowering.h +++ b/llvm/lib/Target/RISCV/RISCVISelLowering.h @@ -464,7 +464,7 @@ class RISCVTargetLowering : public TargetLowering { SmallVectorImpl<Use *> &Ops) const override; bool shouldScalarizeBinop(SDValue VecOp) const override; bool isOffsetFoldingLegal(const GlobalAddressSDNode *GA) const override; - int getLegalZfaFPImm(const APFloat &Imm, EVT VT) const; + std::pair<int, bool> getLegalZfaFPImm(const APFloat &Imm, EVT VT) const; bool isFPImmLegal(const APFloat &Imm, EVT VT, bool ForCodeSize) const override; bool isExtractSubvectorCheap(EVT ResVT, EVT SrcVT, diff --git a/llvm/test/CodeGen/RISCV/double-zfa.ll b/llvm/test/CodeGen/RISCV/double-zfa.ll index 162f33dd5ac9dd2..f159002680bd111 100644 --- a/llvm/test/CodeGen/RISCV/double-zfa.ll +++ b/llvm/test/CodeGen/RISCV/double-zfa.ll @@ -141,23 +141,23 @@ define double @loadfpimm16() { ret double -1.0 } -; Ensure fli isn't incorrectly used for negated versions of numbers in the fli +; Ensure fli isn't directly used for negated versions of numbers in the fli ; table. define double @loadfpimm17() { ; CHECK-LABEL: loadfpimm17: ; CHECK: # %bb.0: -; CHECK-NEXT: lui a0, %hi(.LCPI15_0) -; CHECK-NEXT: fld fa0, %lo(.LCPI15_0)(a0) +; CHECK-NEXT: fli.d fa5, 2.0 +; CHECK-NEXT: fneg.d fa0, fa5 ; CHECK-NEXT: ret ret double -2.0 } -; Ensure fli isn't incorrecty used for negative min normal value. +; Ensure fli isn't directly used for negative min normal value. define double @loadfpimm18() { ; CHECK-LABEL: loadfpimm18: ; CHECK: # %bb.0: -; CHECK-NEXT: lui a0, %hi(.LCPI16_0) -; CHECK-NEXT: fld fa0, %lo(.LCPI16_0)(a0) +; CHECK-NEXT: fli.d fa5, min +; CHECK-NEXT: fneg.d fa0, fa5 ; CHECK-NEXT: ret ret double 0x8010000000000000 } diff --git a/llvm/test/CodeGen/RISCV/float-zfa.ll b/llvm/test/CodeGen/RISCV/float-zfa.ll index 6f9e5ec035224df..52c9ac7333fc518 100644 --- a/llvm/test/CodeGen/RISCV/float-zfa.ll +++ b/llvm/test/CodeGen/RISCV/float-zfa.ll @@ -95,23 +95,23 @@ define float @loadfpimm11() { ret float -1.0 } -; Ensure fli isn't incorrectly used for negated versions of numbers in the fli +; Ensure fli isn't directly used for negated versions of numbers in the fli ; table. define float @loadfpimm12() { ; CHECK-LABEL: loadfpimm12: ; CHECK: # %bb.0: -; CHECK-NEXT: lui a0, 786432 -; CHECK-NEXT: fmv.w.x fa0, a0 +; CHECK-NEXT: fli.s fa5, 2.0 +; CHECK-NEXT: fneg.s fa0, fa5 ; CHECK-NEXT: ret ret float -2.0 } -; Ensure fli isn't incorrecty used for negative min normal value. +; Ensure fli isn't directly used for negative min normal value. define float @loadfpimm13() { ; CHECK-LABEL: loadfpimm13: ; CHECK: # %bb.0: -; CHECK-NEXT: lui a0, 526336 -; CHECK-NEXT: fmv.w.x fa0, a0 +; CHECK-NEXT: fli.s fa5, min +; CHECK-NEXT: fneg.s fa0, fa5 ; CHECK-NEXT: ret ret float 0xb810000000000000 } diff --git a/llvm/test/CodeGen/RISCV/half-zfa-fli.ll b/llvm/test/CodeGen/RISCV/half-zfa-fli.ll index 577b774c27a0b36..281a873235623b3 100644 --- a/llvm/test/CodeGen/RISCV/half-zfa-fli.ll +++ b/llvm/test/CodeGen/RISCV/half-zfa-fli.ll @@ -195,13 +195,13 @@ define half @loadfpimm13() { ret half -1.0 } -; Ensure fli isn't incorrectly used for negated versions of numbers in the fli +; Ensure fli isn't directly used for negated versions of numbers in the fli ; table. define half @loadfpimm14() { ; CHECK-LABEL: loadfpimm14: ; CHECK: # %bb.0: -; CHECK-NEXT: lui a0, 1048572 -; CHECK-NEXT: fmv.h.x fa0, a0 +; CHECK-NEXT: fli.h fa5, 2.0 +; CHECK-NEXT: fneg.h fa0, fa5 ; CHECK-NEXT: ret ; ; ZFHMIN-LABEL: loadfpimm14: @@ -212,12 +212,12 @@ define half @loadfpimm14() { ret half -2.0 } -; Ensure fli isn't incorrecty used for negative min normal value. +; Ensure fli isn't directly used for negative min normal value. define half @loadfpimm15() { ; CHECK-LABEL: loadfpimm15: ; CHECK: # %bb.0: -; CHECK-NEXT: lui a0, %hi(.LCPI14_0) -; CHECK-NEXT: flh fa0, %lo(.LCPI14_0)(a0) +; CHECK-NEXT: fli.h fa5, min +; CHECK-NEXT: fneg.h fa0, fa5 ; CHECK-NEXT: ret ; ; ZFHMIN-LABEL: loadfpimm15: @@ -227,4 +227,3 @@ define half @loadfpimm15() { ; ZFHMIN-NEXT: ret ret half 0xH8400 } - 

[mshockwave]

I have no idea why clang-format will prefer formatting like this...

[asb]

LGTM, good spot that this can be improved. I think the way this patches modifies getLegalZfaFPImm makes sense, keeping logic changes minimal. But there's perhaps room for a follow-up refactoring that exploits knowledge about the table only having one negative constant and so avoids calling itself again.

[mshockwave]

that exploits knowledge about the table only having one negative constant and so avoids calling itself again.

I'm not sure if I understand you correctly, but right now we only recursively call getLegalZfaFPImm with negated FPImm if FPImm is negative. So I believe we've avoided most of the redundant recursive calls.

[asb]

that exploits knowledge about the table only having one negative constant and so avoids calling itself again.

I'm not sure if I understand you correctly, but right now we only recursively call getLegalZfaFPImm with negated FPImm if FPImm is negative. So I believe we've avoided most of the redundant recursive calls.

My thought was just that it might read cleaner to handle the negative input case up-front and avoid the self-invocation. i.e. if the input is negative but isn't -1, then set then negate the APFloat and set NeedsFNeg before proceeding to look up into the table. But tastes vary.