Skip to content

[RISCV] Move the rest of Zfa FLI instruction handling to lowerConstantFP. #109217

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 3 commits into from
Sep 19, 2024

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Sep 18, 2024

We already moved the fneg case. This moves the rest so we can drop the custom isel.

…tFP.

We already moved the fneg case. This moves the rest so we can
drop the custom isel.
@llvmbot
Copy link
Member

llvmbot commented Sep 18, 2024

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

Author: Craig Topper (topperc)

Changes

We already moved the fneg case. This moves the rest so we can drop the custom isel.


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

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp (-30)
  • (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+14-10)
diff --git a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
index f22ab1e5e9d48a..fcd46b5921c4de 100644
--- a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
@@ -889,29 +889,6 @@ 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);
-    if (FPImm >= 0) {
-      unsigned Opc;
-      switch (VT.SimpleTy) {
-      default:
-        llvm_unreachable("Unexpected size");
-      case MVT::f16:
-        Opc = RISCV::FLI_H;
-        break;
-      case MVT::f32:
-        Opc = RISCV::FLI_S;
-        break;
-      case MVT::f64:
-        Opc = RISCV::FLI_D;
-        break;
-      }
-      SDNode *Res = CurDAG->getMachineNode(
-          Opc, DL, VT, CurDAG->getTargetConstant(FPImm, DL, XLenVT));
-
-      ReplaceNode(Node, Res);
-      return;
-    }
 
     bool NegZeroF64 = APF.isNegZero() && VT == MVT::f64;
     SDValue Imm;
@@ -3552,13 +3529,6 @@ bool RISCVDAGToDAGISel::selectScalarFPAsInt(SDValue N, SDValue &Imm) {
 
   MVT VT = CFP->getSimpleValueType(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) >= 0)
-    return false;
-
   MVT XLenVT = Subtarget->getXLenVT();
   if (VT == MVT::f64 && !Subtarget->is64Bit()) {
     assert(APF.isNegZero() && "Unexpected constant.");
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 189fb741f34cd1..fc12c88445200d 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -2268,9 +2268,6 @@ bool RISCVTargetLowering::isFPImmLegal(const APFloat &Imm, EVT VT,
   if (!IsLegalVT)
     return false;
 
-  if (getLegalZfaFPImm(Imm, VT) >= 0)
-    return true;
-
   // Cannot create a 64 bit floating-point immediate value for rv32.
   if (Subtarget.getXLen() < VT.getScalarSizeInBits()) {
     // td can handle +0.0 or -0.0 already.
@@ -5802,22 +5799,29 @@ SDValue RISCVTargetLowering::lowerConstantFP(SDValue Op,
   MVT VT = Op.getSimpleValueType();
   const APFloat &Imm = cast<ConstantFPSDNode>(Op)->getValueAPF();
 
-  if (getLegalZfaFPImm(Imm, VT) >= 0)
-    return Op;
+  // Can this constant be select by a Zfa FLI instruction?
+  bool Negate = false;
+  int Index = getLegalZfaFPImm(Imm, VT);
 
-  if (!Imm.isNegative())
-    return SDValue();
+  // If the constant is negative, try negating.
+  if (Index < 0 && Imm.isNegative()) {
+    Index = getLegalZfaFPImm(-Imm, VT);
+    Negate = true;
+  }
 
-  int Index = getLegalZfaFPImm(-Imm, VT);
+  // If we couldn't find a FLI lowering, fall back to generic code.
   if (Index < 0)
     return SDValue();
 
   // Emit an FLI+FNEG. We use a custom node to hide from constant folding.
   SDLoc DL(Op);
   SDValue Const =
-      DAG.getNode(RISCVISD::FLI, Op, VT,
+      DAG.getNode(RISCVISD::FLI, DL, VT,
                   DAG.getTargetConstant(Index, DL, Subtarget.getXLenVT()));
-  return DAG.getNode(ISD::FNEG, Op, VT, Const);
+  if (!Negate)
+    return Const;
+
+  return DAG.getNode(ISD::FNEG, DL, VT, Const);
 }
 
 static SDValue LowerATOMIC_FENCE(SDValue Op, SelectionDAG &DAG,

dtcxzyw added a commit to dtcxzyw/llvm-codegen-benchmark that referenced this pull request Sep 19, 2024
@dtcxzyw
Copy link
Member

dtcxzyw commented Sep 19, 2024

This patch is not NFC.

; bin/llc -mtriple=riscv64 -mattr=+zfa,+d test.ll -o -
define float @test_float(float %x) nounwind {
entry:
  %cmp = fcmp ult float %x, 0.000000e+00
  %sel = select i1 %cmp, float -5.000000e-01, float 5.000000e-01
  ret float %sel
}

define double @test_double(double %x) nounwind {
entry:
  %cmp = fcmp ult double %x, 0.000000e+00
  %sel = select i1 %cmp, double -5.000000e-01, double 5.000000e-01
  ret double %sel
}

Before:

test_float:                             # @test_float
# %bb.0:                                # %entry
        fmv.w.x fa5, zero
        fle.s   a0, fa5, fa0
        fli.s   fa0, 0.5
        bnez    a0, .LBB0_2
# %bb.1:
        fneg.s  fa0, fa0
.LBB0_2:                                # %entry
        ret

test_double:                            # @test_double
# %bb.0:                                # %entry
        fmv.d.x fa5, zero
        fle.d   a0, fa5, fa0
        fli.d   fa0, 0.5
        bnez    a0, .LBB1_2
# %bb.1:
        fneg.d  fa0, fa0
.LBB1_2:                                # %entry
        ret

After:

test_float:                             # @test_float
# %bb.0:                                # %entry
        fmv.w.x fa5, zero
        fle.s   a0, fa5, fa0
        fli.s   fa0, 0.5
        bnez    a0, .LBB0_2
# %bb.1:
        fneg.s  fa0, fa0
.LBB0_2:                                # %entry
        ret

.LCPI1_0:
        .quad   0x3fe0000000000000              # double 0.5
        .quad   0xbfe0000000000000              # double -0.5
        .text
        .globl  test_double
        .p2align        2
        .type   test_double,@function
test_double:                            # @test_double
# %bb.0:                                # %entry
        fmv.d.x fa5, zero
        fle.d   a0, fa5, fa0
        xori    a0, a0, 1
        slli    a0, a0, 3
        lui     a1, %hi(.LCPI1_0)
        addi    a1, a1, %lo(.LCPI1_0)
        add     a0, a1, a0
        fld     fa0, 0(a0)
        ret

@topperc
Copy link
Collaborator Author

topperc commented Sep 19, 2024

This patch is not NFC.

; bin/llc -mtriple=riscv64 -mattr=+zfa,+d test.ll -o -
define float @test_float(float %x) nounwind {
entry:
  %cmp = fcmp ult float %x, 0.000000e+00
  %sel = select i1 %cmp, float -5.000000e-01, float 5.000000e-01
  ret float %sel
}

define double @test_double(double %x) nounwind {
entry:
  %cmp = fcmp ult double %x, 0.000000e+00
  %sel = select i1 %cmp, double -5.000000e-01, double 5.000000e-01
  ret double %sel
}

Before:

test_float:                             # @test_float
# %bb.0:                                # %entry
        fmv.w.x fa5, zero
        fle.s   a0, fa5, fa0
        fli.s   fa0, 0.5
        bnez    a0, .LBB0_2
# %bb.1:
        fneg.s  fa0, fa0
.LBB0_2:                                # %entry
        ret

test_double:                            # @test_double
# %bb.0:                                # %entry
        fmv.d.x fa5, zero
        fle.d   a0, fa5, fa0
        fli.d   fa0, 0.5
        bnez    a0, .LBB1_2
# %bb.1:
        fneg.d  fa0, fa0
.LBB1_2:                                # %entry
        ret

After:

test_float:                             # @test_float
# %bb.0:                                # %entry
        fmv.w.x fa5, zero
        fle.s   a0, fa5, fa0
        fli.s   fa0, 0.5
        bnez    a0, .LBB0_2
# %bb.1:
        fneg.s  fa0, fa0
.LBB0_2:                                # %entry
        ret

.LCPI1_0:
        .quad   0x3fe0000000000000              # double 0.5
        .quad   0xbfe0000000000000              # double -0.5
        .text
        .globl  test_double
        .p2align        2
        .type   test_double,@function
test_double:                            # @test_double
# %bb.0:                                # %entry
        fmv.d.x fa5, zero
        fle.d   a0, fa5, fa0
        xori    a0, a0, 1
        slli    a0, a0, 3
        lui     a1, %hi(.LCPI1_0)
        addi    a1, a1, %lo(.LCPI1_0)
        add     a0, a1, a0
        fld     fa0, 0(a0)
        ret

Thanks. I'll take look.

dtcxzyw added a commit to dtcxzyw/llvm-codegen-benchmark that referenced this pull request Sep 19, 2024
Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

Copy link
Contributor

@asb asb left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -5802,22 +5802,29 @@ SDValue RISCVTargetLowering::lowerConstantFP(SDValue Op,
MVT VT = Op.getSimpleValueType();
const APFloat &Imm = cast<ConstantFPSDNode>(Op)->getValueAPF();

if (getLegalZfaFPImm(Imm, VT) >= 0)
return Op;
// Can this constant be select by a Zfa FLI instruction?
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: "select" -> "selected"

@topperc topperc merged commit 079f31c into llvm:main Sep 19, 2024
6 of 8 checks passed
@topperc topperc deleted the pr/zfa-fli branch September 19, 2024 22:16
@llvm-ci
Copy link
Collaborator

llvm-ci commented Sep 19, 2024

LLVM Buildbot has detected a new failure on builder clangd-ubuntu-tsan running on clangd-ubuntu-clang while building llvm at step 6 "test-build-clangd-clangd-index-server-clangd-indexer-check-clangd".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/5612

Here is the relevant piece of the build log for the reference
Step 6 (test-build-clangd-clangd-index-server-clangd-indexer-check-clangd) failure: test (failure)
******************** TEST 'Clangd :: target_info.test' FAILED ********************
Exit Code: 66

Command Output (stderr):
--
RUN: at line 5: rm -rf /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.dir && mkdir -p /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.dir
+ rm -rf /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.dir
+ mkdir -p /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.dir
RUN: at line 7: echo '[{"directory": "/vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.dir", "command": "/vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.dir/armv7-clang -x c++ the-file.cpp -v", "file": "the-file.cpp"}]' > /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.dir/compile_commands.json
+ echo '[{"directory": "/vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.dir", "command": "/vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.dir/armv7-clang -x c++ the-file.cpp -v", "file": "the-file.cpp"}]'
RUN: at line 9: sed -e "s|INPUT_DIR|/vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.dir|g" /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/llvm-project/clang-tools-extra/clangd/test/target_info.test > /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.test.1
+ sed -e 's|INPUT_DIR|/vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.dir|g' /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/llvm-project/clang-tools-extra/clangd/test/target_info.test
RUN: at line 12: sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.test.1 > /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.test
+ sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.test.1
RUN: at line 14: clangd -lit-test < /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.test 2>&1 | /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/bin/FileCheck -strict-whitespace /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.test
+ clangd -lit-test
+ /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/bin/FileCheck -strict-whitespace /vol/worker/clangd-ubuntu-clang/clangd-ubuntu-tsan/build/tools/clang/tools/extra/clangd/test/Output/target_info.test.tmp.test

--

********************


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.

5 participants