[PATCH] D159053: [RISCV] Support select/merge like ops for fp16 vectors when only have Zvfhmin

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:13988
+    // For splat_vector to fp16 vectors when we only have zvfhmin, we promote it
+    // to fp_round(vf32 splat_vector(c)). But the constant folder of getNode
+    // would fold fp_round(vf32 splat_vector(c)) back to vf16 splat_vector(c)
----------------
michaelmaitland wrote:
> jacquesguan wrote:
> > michaelmaitland wrote:
> > > How does this approach compare to making the constant folder of getNode not do the fold when only have zvfhmin?
> > I have no idea about how to make the constant folder of getNode not do the fold when only have zvfhmin if we just want to change the RISCV target specific code. Do you have some advice?
> Here is one idea (using pseudo code):
> 
> ```
> Target::isConstantFoldLegal(InformationAboutFold) { return true; }
> RISCVTarget::isConstantFoldLegal(InformationAboutFold I) override {
>   if (isConstantFoldFromF32ToF16(I) && onlyHaveZvfhmin())
>     return false;
>   return true;
> }
> 
> ConstantFolder::fold(MachineInstr NotFoldedYet, InformationAboutFold I) {
>   if (Target.isConstantFoldLegal(I))
>     return NotFoldedYet;
>   return doFold(NotFoldedYet);
> }
> ```
> 
> The comment in the code points out that the current solution is a work around to avoid a problem that would have been introduced by the constant folder. If we solve it in the constant folder then we avoid needing to do a work around and allow the lowering to happen at the normal time.
> 
> That being said, I'm not confident that there is anything wrong with lowering early since I am not an expert in this area. I wonder if you have any thoughts (or @craig.topper when he is back from vacation).
I think I'd rather see this implemented as a custom lowering of SPLAT_VECTOR rather than a combine. That likely means we need to manually implement the promotion of SPLAT_VECTOR for the non-constant case in the same lowering since we can't use Promote for setOperationAction, but that doesn't seem like a blocker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159053