Page MenuHomePhabricator

[SelectionDAG] When scalarizing vselect, don't assert on a legal cond operand.
ClosedPublic

Authored by eladcohen on Aug 9 2017, 4:51 AM.

Details

Summary

When scalarizing the result of a vselect, the legalizer currently expects
to already have scalarized the operands. While this is true for the true/false
operands (which have the same type as the result), it is not case for the
condition operand. On X86 AVX512, v1i1 is legal - this leads to operations such
as '< N x type> vselect < N x i1> < N x type> < N x type>' where < N x type > is
illegal to hit an assertion during the scalarization.

The handling is similar to r205625.
This also exposes the fact that (v1i1 extract_subvector) should be legal
and selectable on AVX512 - We do this by custom lowering to vector_extract_elt.
This still leaves us in some cases with redundant dag nodes which will be
combined in a separate soon to come patch.

This fixes pr33349.

Diff Detail

Repository
rL LLVM

Event Timeline

eladcohen created this revision.Aug 9 2017, 4:51 AM
RKSimon accepted this revision.Aug 9 2017, 7:46 AM

LGTM, with one minor.

PS - nothing to do with this patch but those mask vector shifts are awful.

lib/Target/X86/X86ISelLowering.cpp
14538 ↗(On Diff #110361)

Maybe better to assert that we only got here with AVX512:

if (ResVT == MVT::v1i1) {
  assert(Subtarget.hasAVX512() && "Boolean EXTRACT_SUBVECTOR requires AVX512")
  ...
This revision is now accepted and ready to land.Aug 9 2017, 7:46 AM
eladcohen marked an inline comment as done.Aug 9 2017, 8:07 AM

LGTM, with one minor.

PS - nothing to do with this patch but those mask vector shifts are awful.

Thanks for the review!

About the shifts - in the test we can see redundant shifts (due to "extract_elt (extract_subvector X)" ) :

; SKX-NEXT:    kshiftlw $15, %k0, %k1
; SKX-NEXT:    kshiftrw $15, %k1, %k1
; SKX-NEXT:    kshiftlw $15, %k1, %k1
; SKX-NEXT:    kshiftrw $15, %k1, %k1

I'm going to upload a dag combine for

vextract (v1iX extract_subvector(vNiX, Idx)) -> vextract(vNiX,Idx)

which will eliminate the redundant shifts.

About the usage of shifts for extracting the bit in general we have: https://bugs.llvm.org/show_bug.cgi?id=33692 - so that's at least one way we can improve this I guess.

eladcohen updated this revision to Diff 110398.Aug 9 2017, 8:08 AM

Fixed Simon's comment.

RKSimon accepted this revision.Aug 9 2017, 8:19 AM

Thanks, LGTM

This revision was automatically updated to reflect the committed changes.