[PATCH] D66095: [InstCombine] canonicalize a scalar-select-of-vectors to vector select

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D66095#1631547 <https://reviews.llvm.org/D66095#1631547>, @ABataev wrote:

> In D66095#1631524 <https://reviews.llvm.org/D66095#1631524>, @spatel wrote:
>
> > In D66095#1631501 <https://reviews.llvm.org/D66095#1631501>, @ABataev wrote:
> >
> > > In D66095#1631495 <https://reviews.llvm.org/D66095#1631495>, @spatel wrote:
> > >
> > > > In D66095#1630408 <https://reviews.llvm.org/D66095#1630408>, @ABataev wrote:
> > > >
> > > > > What about very long vectors that do no fit into single register? Is it cost effective for such vectors too?
> > > >
> > > >
> > > > We would split the long vectors into values that fit the target registers in the backend. At that point, the target can decide if N vector selects are better or worse than a transfer to scalar compare and branch. As @lebedev.ri mentioned, we're missing that logic in SDAG, but given that this transform produces the better code for the default case, I don't think we need to make this patch dependent on backend fixups.
> > >
> > >
> > > But it adds extra cost for vectors splitting. Maybe limit the size of the vectors in the patch?
> >
> >
> > How would we do that? This is canonicalization, so we are not using any target-dependent information here. Presumably, whoever or whatever created the illegal vectors in the first place knows that codegen will have to alter the those ops to create legal code, so that will be handled by a pass that has a cost model.
>
>
> It means that you can make a transformation that may be less cost-effective than the original code.


I agree with @spatel, there is no TTI in InstCombine, neither should there be.

In general, i do think this fold is the opposite from what the correct canonicalization is,
but as @spatel notes, this fix clearly results in better results right as of this moment.
Things can be readjusted later.


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

https://reviews.llvm.org/D66095