[PATCH] D153848: [RISCV] Apply promotion for f16 vector ops when only have zvfhmin.

[Michael Maitland via Phabricator via llvm-commits]

michaelmaitland accepted this revision.
michaelmaitland added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:5508
   case ISD::SINT_TO_FP:
   case ISD::UINT_TO_FP:
+    if (Op.getValueType().isVector() &&
----------------
jacquesguan wrote:
> michaelmaitland wrote:
> > Why don't we reuse the logic below? 
> > 
> > ```
> > // Widening
> > if (IsInt2FP) {
> >         // Do a regular integer sign/zero extension then convert to float.
> > // Narrowing 
> > if (IsInt2FP) {
> >         // One narrowing int_to_fp, then an fp_round.
> > ```
> > 
> > `narrowing int_to_fp then an fp_round` seems to be what we're doing here right?
> Thanks for advice. I think it would make the logic a little diffcult to read. If we want to reuse this function, we should merge it into this scope.
> ```
>     if (SrcEltSize > (2 * EltSize)) {
>       if (IsInt2FP) {
>       }
>     }
> ```
> But actually we not only have i64->f16 but also i32 i16 and even i8 ->f16, the  condition of this if statement will become complict.
> ```
>     if (SrcEltSize > (2 * EltSize) || ZvfhminPromote) {
>       if (IsInt2FP) {
>       }
>     }
> ```
> And the other part that lower FP2Int should also should exculde ZvfhminPromote logic.
> So should we trade servel lines of code with clear logic?
> I think it would make the logic a little diffcult to read. So should we trade servel lines of code with clear logic?

I will defer to you to decide what you think is best. If you think it will make it more difficult to read, I am happy to keep as is. I only brought it up because I do not see `[[fallthrough]]` used very often. But since it leads to easier to read code, I support your decision.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153848/new/

https://reviews.llvm.org/D153848