[PATCH] D90247: [AArch64] Add legalizations for VECREDUCE_SEQ_FADD

[Nikita Popov via Phabricator via llvm-commits]

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

LGTM with the default action adjusted...



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4909
+    break;
+  }
+
----------------
cameron.mcinally wrote:
> nikic wrote:
> > Again, this is a recurring pattern, so I have extracted a `SelectionDAG::getNeutralElement()` API for this. The invocation should be:
> > 
> > ```
> > SDValue NeutralElem = DAG.getNeutralElement(
> >     ISD::getVecReduceBaseOpcode(N->getOpcode()), dl, ElemVT, Flags);
> > ```
> I improvised a little here for readability. Is that okay with you, @nikic?
> 
> Although, I wonder if getting the BaseOpc is a necessary step here. Did you consider just adding VECREDUCE_* cases to `DAG.getNeutralElement(...)`?
> I improvised a little here for readability. Is that okay with you, @nikic?

Sure!

> Although, I wonder if getting the BaseOpc is a necessary step here. Did you consider just adding VECREDUCE_* cases to `DAG.getNeutralElement(...)`?

Not strictly necessary, but I personally found it a bit more elegant to not repeat the full VECREDUCE list another time here.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:929
       setOperationAction(ISD::VECREDUCE_FMIN, VT, Custom);
+      setOperationAction(ISD::VECREDUCE_SEQ_FADD, VT, Expand);
     }
----------------
Rather than explicitly marking them as expanded in targets, VECREDUCE_SEQ_FADD should be marked as default expanded using the list at https://github.com/llvm/llvm-project/blob/aa1c6b79878475d61c90c0d4c2af17312242f18e/llvm/lib/CodeGen/TargetLoweringBase.cpp#L722, to stay consistent with other VECREDUCE opcodes.


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

https://reviews.llvm.org/D90247