[PATCH] D75062: [VectorCombine] make cost calc consistent for binops and cmps

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D75062#1889864 <https://reviews.llvm.org/D75062#1889864>, @lebedev.ri wrote:

> In D75062#1889774 <https://reviews.llvm.org/D75062#1889774>, @spatel wrote:
>
> > In D75062#1889707 <https://reviews.llvm.org/D75062#1889707>, @lebedev.ri wrote:
> >
> > > > We have the reverse transform with target hook in DAGCombiner already -- scalarizeExtractedBinop() -- so that provides a later opportunity to invert (potentially with a more accurate cost model).
> > >
> > > I'm a little bit lost there.
> > >  Unless i'm not reading it right, even if the `TLI.shouldScalarizeBinop(Vec)` returns `true`,
> > >  the code only scalarizes if at least one of the vector operands is constant.
> >
> >
> > Oops, I misread it - I saw an x86 FP example like:
> >
> >   define float @fadd_extract(<4 x float> %x, <4 x float> %y) {
> >     %a = fadd <4 x float> %x, %y
> >     %r = extractelement <4 x float> %a, i32 0
> >     ret float %r
> >   }
> >   
> >
> > ...but that's handled by the x86-specific scalarizeExtEltFP().
> >
> > I can try to make scalarizeExtractedBinop() stronger, but I'll need to find a target/example where it is a win (the extracts need to be free, or the vector op needs to be pretty expensive?). Or we just see if this causes wins or regressions for anyone as-is?
>
>
> I think that either `scalarizeExtractedBinop()` should actually be able to do the inverse transform,
>  or this patch's description shouldn't say `scalarizeExtractedBinop()` can do inverse transform.


Changing the description/comments is easy. :)
I'm willing to see if that works out. And if there are problems, I can adjust the DAGCombiner code.


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

https://reviews.llvm.org/D75062