[PATCH] D122918: [RISCV][CodeGen] Support float-arith in Zfinx

[Shao-Ce SUN via Phabricator via llvm-commits]

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