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

[Jianjian Guan via Phabricator via llvm-commits]

jacquesguan added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:5508
   case ISD::SINT_TO_FP:
   case ISD::UINT_TO_FP:
+    if (Op.getValueType().isVector() &&
----------------
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?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:394
     setOperationAction(ISD::FREM, MVT::bf16, Promote);
     // FIXME: Need to promote bf16 FCOPYSIGN to f32, but the
     // DAGCombiner::visitFP_ROUND probably needs improvements first.
----------------
michaelmaitland wrote:
> Does this need to be fixed as a part of this patch?
I don't think so this part should be involved into this patch. This fixme is about bf16 not fp16.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153848