This is an archive of the discontinued LLVM Phabricator instance.

[LegalizeDAG] Add generic vector CTPOP expansion (PR32655)
ClosedPublic

Authored by RKSimon on Oct 14 2018, 9:16 AM.

Details

Summary

This patch adds support for expanding vector CTPOP instructions and removes the x86 'bitmath' lowering which replicates the same expansion.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Oct 14 2018, 9:16 AM
efriedma added inline comments.Oct 15 2018, 1:02 PM
lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1099

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.

RKSimon added inline comments.Oct 15 2018, 2:15 PM
lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1099

Sounds good (and should help D52965 as well) - do we have existing examples of this?

efriedma added inline comments.Oct 15 2018, 2:46 PM
lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1099

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.

RKSimon updated this revision to Diff 170726.Oct 23 2018, 12:34 PM

Updated vector CTPOP expansion to use TargetLowering::expandCTPOP.

We still have the issue with x86 bitcast'd logic operations preventing reassociation.

RKSimon updated this revision to Diff 171396.Oct 27 2018, 4:01 AM

rebase - now that D53268 has landed the X86 code diffs have gone

RKSimon retitled this revision from [LegalizeDAG] Add generic vector CTPOP expansion (PR32655) (WIP) to [LegalizeDAG] Add generic vector CTPOP expansion (PR32655).Oct 27 2018, 4:02 AM
RKSimon edited the summary of this revision. (Show Details)Oct 29 2018, 10:58 AM
efriedma added inline comments.Oct 31 2018, 3:22 PM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
4153

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.

4160

I'm sort of on the fence about whether this check should be here, or in VectorLegalizer::ExpandCTPOP. Either way works, I guess.

RKSimon added inline comments.Nov 1 2018, 2:10 AM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
4153

This is a direct copy from the LegalizeDAG version - I can change this to an early out if you'd prefer

4160

I agree its awkward, I went for putting it close to the actual codegen in the hope that any changes will stay synced up.

RKSimon updated this revision to Diff 172123.Nov 1 2018, 7:40 AM

Early out from TargetLowering::expandCTPOP for unsupported integer bitwidths

This revision is now accepted and ready to land.Nov 1 2018, 10:52 AM
This revision was automatically updated to reflect the committed changes.