Paths
| Differential D141560
[RISCV][CodeGen] Add codegen pattern for FLI instruction in experimental zfa extension ClosedPublic Authored by joshua-arch1 on Jan 11 2023, 5:49 PM.
Details Summary This patch implements experimental support for the RISCV Zfa extension as specified here: https://github.com/riscv/riscv-isa-manual/releases/download/draft-20221119-5234c63/riscv-spec.pdf, Ch. 25. This extension has not been ratified. Once ratified, it'll move out of experimental status. This change adds codegen support for load-immediate instructions (fli.s/fli.d/fli.h).
Diff Detail Event TimelineComment Actions Since the assmebly support patch for Zfa instructions except FLI has been accepted, could anyone please review this codegen patch?
Comment Actions
I'd encourage you to land the underlying approved review. I can't speak for others, but I certainty prioritize review for changes which have fewer outstanding blocking issues. You could also split this into an FLI and non-FLI part with the same reasoning. Comment Actions
Different from assembly support, I think there is nothing special with the codegen pattern for FLI instruction. It's unnecessory to split the codegen patch into an FLI and non-FLI part.
Comment Actions
The suggestion to split is to unblock parts of the patch. We would rather have small pieces make progress than stalling a whole patch when one part of it that can be isolated needs additional work. Smaller patches are much easier to review and re-review.
Comment Actions
I see. I will split these patch into three small ones. One for FLI, one for FCVMOD and another for other instructions. Comment Actions Codegen patch for instructions except FLI and FCVTMOD has been commited. https://reviews.llvm.org/D143982 Comment Actions
Important English point here. The patch has been *posted* not *committed*. Saying it's committed means that it was landed to the git repository. That appears to not be what you meant, and was actively confusing in this case. joshua-arch1 retitled this revision from [RISCV][CodeGen] Add codegen pattern for experimental zfa extension to [RISCV][CodeGen] Add codegen pattern for FLI instruction in experimental zfa extension.Feb 16 2023, 4:39 AM
Comment Actions Ping.
This revision is now accepted and ready to land.Feb 19 2023, 6:37 PM Comment Actions
This CodeGen patch depends on some functions in https://reviews.llvm.org/D140460. This revision was landed with ongoing or failed builds.Mar 6 2023, 10:27 PM Closed by commit rGada264146067: [RISCV][CodeGen] Add codegen pattern for FLI instruction in experimental zfa… (authored by joshua-arch1). · Explain Why This revision was automatically updated to reflect the committed changes.
Comment Actions Anyone knows how to generate FLI from C-code? If I compile the following program, I cannot get FlI. ConstantFP will be converted to Constant in DAG. void foo_double64 ()
{
volatile double a;
a = 0.0625;
}
Comment Actions
You just need to use it to do some floating point arithmetic void foo_double64 (double x)
{
volatile double a;
a = x + 0.0625;
}or return a floating point value double foo_double64 ()
{
return 0.0625;
}
Comment Actions
Is that because we cannot directly store a ConstantFP in DAG? Comment Actions
The change is done in DAGCombiner::replaceStoreOfFPConstant. I think the idea is that FP constants are usually harder to create in registers than integer constants. Also CPUs usually have more integer resources than FP resources. It doesn't look like it can be disabled currently. I think all of the constants FLI handles can be done in 1 or 2 integer instructions so I'm not very concerned about this. It was more than that I might be concerned. Comment Actions
Will one FLI instruction have less cycles and perform better than two integer instructions? Comment Actions
Maybe, but it will probably be microarchitecture dependent. Delaying a store by an extra cycle doesn't seem like a big deal. Other instructions don't directly depend on a store except later loads. The compiler would often be able to see the load is to the same location as the store and forward the data without the load. If we're storing the same value in a loop we should be hoisting the 2 integer instructions out of the loop. So they wouldn't be executed many times. Comment Actions
Even if we disable replaceStoreOfFPConstant in DAGCombiner, FP constant will still be converted to integer in OptimizeFloatStore() when legalizing. Comment Actions
I can see that it won't ever do it for f32 on rv64 because it doesn't consider the possibility of using an i32 store when i32 isn't a legal type. Beyond that I would need to see your test cases.
Revision Contents
Diff 488444 llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
llvm/lib/Target/RISCV/RISCVISelLowering.h
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td
llvm/test/CodeGen/RISCV/double-zfa.ll
llvm/test/CodeGen/RISCV/float-zfa.ll
llvm/test/CodeGen/RISCV/half-zfa.ll
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-bitcast.ll
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This description is incorrect. The FCVTMOD instructions do not saturate.