When using clang option -frapping-math, RISC-V backend crash.
This patch enable Lowering for strictp in riscv to get rid of crash.
Details
- Reviewers
luismarques asb lenary
Diff Detail
Event Timeline
llvm/test/CodeGen/RISCV/fp-strict.ll | ||
---|---|---|
3–6 | It's probably a good idea to use the hardfloat ABI, to cut down on the amount of GPR to FPR conversion instructions, etc. | |
132 | Shouldn't this test also have an fcmps oeq case, showing the proper behavior in terms of NaN signaling, by using the correct RISC-V instruction for that? I haven't yet delved into the lowerSETCC implementation but it seems that having such tests here would clarify if this is correctly implemented. |
llvm/test/CodeGen/RISCV/fp-strict.ll | ||
---|---|---|
3–6 | thanks , will consider it in next diff update. |
Disclaimer: I haven't swotted up on these constrained intrinsics to review the proposed lowering yet, but added a quick note on the setOperationAction calls. I agree with Luis the test cases would probably be easier to read if using the hard float ABI. I think you're lacking any test coverage for f32 as well.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
214 | Should the instances of MVT::i64 be XLenVT? I also would have imagined it would make sense for these setOperationAction calls to be guarded on hasStdExtF/hasStdExtD appropriately. Is that not the case here? |
llvm/lib/Target/RISCV/RISCVISelLowering.h | ||
---|---|---|
212 | Don't delete blank line here. |
llvm/lib/Target/RISCV/RISCVISelLowering.h | ||
---|---|---|
212 | ok, will fix it in next update |
You will need to rebase past D80952, which made constrained fp a target opt-in - though in all honesty, with that patch, this patch is less of a priority as the compiler will not crash any more.
I still don't know how we (the reviewers) should work out whether the lowering of the constrained fp operators is "correct" or not, which is what's holding up this patch anyway.