[PATCH] D127710: [SelectionDAG] Enable WidenVecOp_VECREDUCE_SEQ for scalable vector

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:6152
+  if (WideVT.isScalableVector()) {
+    unsigned GCD = greatestCommonDivisor(OrigElts, WideElts);
+    EVT SplatVT = EVT::getVectorVT(*DAG.getContext(), ElemVT,
----------------
Jimerlife wrote:
> craig.topper wrote:
> > sdesmalen wrote:
> > > I may be missing something here, but I'm not sure I understand the need for a `GCD` or for a loop that generates `WideVT.getVectorMinNumElements()`  `INSERT_SUBVECTOR`s. Should this code not just insert `VecOp` into a `splat(NeutralElem)` of type `WideVT` using INSERT_SUBVECTOR?
> > > 
> > > i.e.
> > > 
> > >    vecreduce_seq(<vscale x 10 x half> op)
> > >        <=>
> > >    vecreduce_seq(
> > >        insert_subvector(op, <vscale x 16 x half> splat(%neutral.elem), /*idx=*/0))
> > > 
> > > where `<vscale x 10 x half>` is `OrigVT` and `<vscale x 16 x half>` is `WideVT`.
> > I think you meant `insert_subvector(<vscale x 16 x half> splat(%neutral.elem), op, /*idx=*/0)`. `op` would be the subvector so should be the second argument.
> > 
> > That would need to go through WidenVecOp_INSERT_SUBVECTOR, but it will fail because `InVec` isn't undef.
> If we do like this `vecreduce_seq(insert_subvector(<vscale x 16 x half> splat, vecop, /*idx=*/0))`, that would go through WidenVecOp_INSERT_SUBVECTOR because of `vecop` need to widen. 
> But now, WidenVecOp_INSERT_SUBVECTOR only support insert subvector into undefined vector.
> 
Thanks, that makes sense.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:6153
+    unsigned GCD = greatestCommonDivisor(OrigElts, WideElts);
+    EVT SplatVT = EVT::getVectorVT(*DAG.getContext(), ElemVT,
+                                   ElementCount::getScalable(GCD));
----------------
For SVE there is still the problem that the type action of SplatVT may be `TypeWiden` as well, because we haven't implemented all support for nxv1 types yet. The approach taken here seems valid though, it just means that we can't have the same tests for SVE yet.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vreductions-fp-sdnode.ll:1081
+
+define half @vreduce_ord_fadd_nxv6f16(<vscale x 6 x half> %v, half %s) {
+; CHECK-LABEL: vreduce_ord_fadd_nxv6f16:
----------------
Could you also add `@vreduce_ord_fadd_nxv6f16` and `@vreduce_ord_fadd_nxv10f16`  to llvm/test/CodeGen/AArch64/vecreduce-fadd-legalization-strict.ll as well ? (and then just re-generate the CHECK lines using the `update_lcc_test_checks.py` script)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127710