[PATCH] D53258: [LegalizeDAG] Add generic vector CTPOP expansion (PR32655) (WIP)

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1091
+      TLI.isOperationLegalOrCustom(ISD::SRL, VT) &&
+      (NumBitsPerElt == 8 || TLI.isOperationLegalOrCustom(ISD::MUL, VT)) &&
+      TLI.isOperationLegalOrCustomOrPromote(ISD::OR, VT) &&
----------------
RKSimon wrote:
> efriedma wrote:
> > It should be possible to avoid using MUL if it isn't legal. Not sure how much work that would be to fix.
> > 
> > It's generally not a good idea to push vector legalization work to LegalizeDAG; there's a separate legalization stage for vectors precisely because LegalizeDAG can't unroll arbitrary vectors.  (This is the issue you mentioned with ARM support for v2i64 multiply.)  It would be better to factor out the expansion routine into a separate function, and call it from both VectorLegalizer and LegalizeDAG.
> Sounds good (and should help D52965 as well) - do we have existing examples of this?
Not sure we have great examples of sharing between LegalizeDAG and LegalizeVectorOps, specifically, but we have some shared lowering routines in TargetLowering.h that are used in various places.


Repository:
  rL LLVM

https://reviews.llvm.org/D53258