[RISCV] Teach RISCVTargetLowering::isFPImmLegal about fli+fneg #149075
Conversation
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.
|
@llvm/pr-subscribers-backend-risc-v Author: Alex Bradbury (asb) ChangesThere was a mismatch between isFPImmlegal and the cases that are handled by lowerConstantFP. isFPImmLegal didn't check for the case where we support 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:
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.
|
|
I removed this in #108316. isLegalFPImm is not supposed to be in sync with lowerConstantFP. lowerConstantFP handles things that aren't legal. |
|
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 But looking closer, LegalizeDAG.cpp only checks |
There was a mismatch between isFPImmlegal and the cases that are handled by lowerConstantFP. isFPImmLegal didn't check for the case where we support
fliof 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.