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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

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.


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

https://reviews.llvm.org/D75062