[PATCH] D122918: [RISCV][CodeGen] Support float-arith in Zfinx
Shao-Ce SUN via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 13 03:53:40 PDT 2022
sunshaoce added a comment.
In D122918#3447845 <https://reviews.llvm.org/D122918#3447845>, @hughperkins wrote:
> Ok, I've since learned that this PR doesnt handle load/store. Only arithmetic.
>
> However note that immediate float arithmetic also seems to be missing, e.g. the following crashes
>
> define noundef float @_Z4funcf(float noundef %0) #0 {
> %2 = fadd float %0, 3.000000e+00
> ret float %2
> }
>
> Compiled with:
>
> bin/llc --march riscv32 -mattr +zfinx test_fadd_imm.ll
>
> Gives:
>
> LLVM ERROR: Cannot select: t15: f32,ch = load<(load (s32) from constant-pool)> t0, t19, undef:i32
> t19: i32 = ADDI t18, TargetConstantPool:i32<float 3.000000e+00> 0 [TF=3]
> t18: i32 = LUI TargetConstantPool:i32<float 3.000000e+00> 0 [TF=4]
> t16: i32 = TargetConstantPool<float 3.000000e+00> 0 [TF=4]
> t17: i32 = TargetConstantPool<float 3.000000e+00> 0 [TF=3]
> t14: i32 = undef
> In function: _Z4funcf
>
> (but compiles ok with `+f`)
Thanks, I'll try to fix it.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoF.td:515
/// Float arithmetic operations
+defm : PatFprFprDynFrm_m<any_fadd, FADD_S, FINX>;
+defm : PatFprFprDynFrm_m<any_fsub, FSUB_S, FINX>;
----------------
hughperkins wrote:
> I think it's confusing that `FINX` in this context means `F` and `F_INX`. I feel it might be good to make more explicit that `FINX` includes `F`. How to do that is an open question. I'd suggest `FFINX`, but `FFINX` is already taken to mean `FF` with `FF_INX`. Maybe we need an explicit acronym to mean 'F and F_INX', like e.g. 'ALLF'. Or perhaps use a verbose form like 'F_AND_FINX'? (If it was my own decision I'd go for the explicit `F_AND_FINX`)
Your suggestion is very good, but I think `FINX` should be replaced with a shorter name because it appears as a parameter, eg `F_INX`. And consider changing the name of `F_INX` to `FprINX`. Would you like to submit a new patch about this?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122918/new/
https://reviews.llvm.org/D122918
More information about the llvm-commits
mailing list