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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4219
   assert(VT.isInteger() && Len <= 128 && Len % 8 == 0 &&
          "CTPOP not implemented for this type.");
 
----------------
efriedma wrote:
> It doesn't seem safe to assert that all legal integer vector types (or, more generally, all legal integer types) have size between i8 and i128.
This is a direct copy from the LegalizeDAG version - I can change this to an early out if you'd prefer


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4226
+                        (Len != 8 && !isOperationLegalOrCustom(ISD::MUL, VT)) ||
+                        !isOperationLegalOrCustomOrPromote(ISD::AND, VT)))
+    return false;
----------------
efriedma wrote:
> I'm sort of on the fence about whether this check should be here, or in VectorLegalizer::ExpandCTPOP.  Either way works, I guess.
I agree its awkward, I went for putting it close to the actual codegen in the hope that any changes will stay synced up.


Repository:
  rL LLVM

https://reviews.llvm.org/D53258