[RISCV] Teach RISCVTargetLowering::isFPImmLegal about fli+fneg

[asb]

There was a mismatch between isFPImmlegal and the cases that are handled by lowerConstantFP. isFPImmLegal didn't check for the case where we support fli of a negated constant (and so can lower to fli+fneg). This has very minimal impact (42 insertion, 47 deletions across an rv22u64_zfa llvm-test-suite build including SPEC CPU 2017) but is added here for completeness.

---

As for testing, from what I can see we don't have consistent testing of the logic here. If you're happy this is trivial enough it can land as-is. If we do want to add tests here, this is a case where I'd probably advocate for adding C++ unit tests for isFPImmLegal directly rather than trying to test it indirectly in a potentially brittle way. But welcome your feedback/suggestions.

[llvmbot]

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

Author: Alex Bradbury (asb)

Changes

There was a mismatch between isFPImmlegal and the cases that are handled by lowerConstantFP. isFPImmLegal didn't check for the case where we support fli of a negated constant (and so can lower to fli+fneg). This has very minimal impact (42 insertion, 47 deletions across an rv22u64_zfa llvm-test-suite build including SPEC CPU 2017) but is added here for completeness.

---

As for testing, from what I can see we don't have consistent testing of the logic here. If you're happy this is trivial enough it can land as-is. If we do want to add tests here, this is a case where I'd probably advocate for adding C++ unit tests for isFPImmLegal directly rather than trying to test it indirectly in a potentially brittle way. But welcome your feedback/suggestions.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+4)

diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index de830666d89b8..b53f2808eeb41 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -2319,6 +2319,10 @@ bool RISCVTargetLowering::isFPImmLegal(const APFloat &Imm, EVT VT,
   if (getLegalZfaFPImm(Imm, VT) >= 0)
     return true;
 
+  // Some constants can be produced by fli+fneg.
+  if (Imm.isNegative() && 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.

[topperc]

I removed this in #108316. isLegalFPImm is not supposed to be in sync with lowerConstantFP. lowerConstantFP handles things that aren't legal.

[asb]

Thanks for the reference. How would you define "legal" constants for isFPImmLegal because by my reading of the function, it's currently counting anything that can be handled with two instructions (e.g. -0.0 via fmv x0 then fneg, and FP values with bit patterns that can be loaded with a single int instruction then fmv).

[topperc]

Thanks for the reference. How would you define "legal" constants for isFPImmLegal because by my reading of the function, it's currently counting anything that can be handled with two instructions (e.g. -0.0 via fmv x0 then fneg, and FP values with bit patterns that can be loaded with a single int instruction then fmv).

I thought "legal" was anything that was handled by the code in case ISD::ConstantFP in RISCVISelDAGToDAG.cpp. Which would be anything that is "legal" to reach isel.

But looking closer, LegalizeDAG.cpp only checks isLegalFPImm in ExpandNode which would be after LowerConstantFP gets called. So maybe we can say things in LowerConstantFP are "legal".

[asb]

But looking closer, LegalizeDAG.cpp only checks isLegalFPImm in ExpandNode which would be after LowerConstantFP gets called. So maybe we can say things in LowerConstantFP are "legal".

Looking at how it's used from DAGCombiner that had been my expectation. Support for selecting the FP value "natively" is a bit debatable on RISC-V, but I think "will be generated cheaply using a small number of instructions rather than loaded from the constant pool" is reasonable, and seems consistent with how that function is implemented today (other than missing the fli+fneg case).

As noted in the commit message, this has almost no impact on llvm-test-suite. But I'd still like to land it for completeness and consistency (and of course the chance of unexpected negative impact seems negligible). What do you think?

[topperc]

LGTM. I think I'm ok with this. But I want to note that this only works because LowerConstantFP creates a RISCVISD::FLI node. Which it does because visitFNEG will always converts ConstantFP+fneg into a ConstantFP of the negated value without checking isFPImmLegal. The RISCVISD::FLI hides the sequence from constant folding.

If we were to ever make visitFNEG obey isFPImmLegal and change LowerConstantFP to emit ConstantFP+FNEG instead of FLI+FNEG, we would infinite loop.