[PATCH] D55780: [X86] Create PSUBUS from (add (umax X, C), -C)

Tue Dec 18 07:51:55 PST 2018

andreadb added a comment.

In D55780#1334596 <https://reviews.llvm.org/D55780#1334596>, @spatel wrote:

> In D55780#1334563 <https://reviews.llvm.org/D55780#1334563>, @andreadb wrote:
>
> > In D55780#1334383 <https://reviews.llvm.org/D55780#1334383>, @spatel wrote:
> >
> > > This change handles psubus cases with no undefs, so LGTM. 
> > >  Depending on how the patches land, we could add the undef capability and tests for it directly here or make that an enhancement.
> > >  But as I mentioned, I don't think this patch alone is enough to close PR40053 because that's at least 2 independent bugs in 1 report.
> >
> >
> > Don't get me wrong: I am not suggesting that we shouldn't commit this patch.
> >
> > I just wanted to point out that it is not true that all the psubus cases are fixed as you wrote. To fully fix psubus we need to make sure that we match UMAX even in the presence of undefs.
> >  As I wrote in my previous comment, if you slightly change the example from the bugzilla, then we no longer get a UMAX in the DAG, and we lose this optimization (see below):
> >
> >   unsigned long long test_sub_2(__m128i x) {
> >       __m128i c = _mm_set1_epi8(70);
> >       return _mm_subs_epu8(x, c)[0];
> >   }
> >
>
>
> Ah, sorry I forgot to address that example. I filed it here:
>  https://bugs.llvm.org/show_bug.cgi?id=40083

No problem. Thanks for raising that bug!

> And I agree, we're missing many saturating math patterns. I started looking into that with:
>  https://bugs.llvm.org/show_bug.cgi?id=14613
>  ...but now that we have IR intrinsics and DAG nodes for saturating math, I think it needs to be revisited so we use those ops.

As Simon wrote, we could wait for D55822 <https://reviews.llvm.org/D55822>, so that we get support for undefs too. That being said, I am okay even if this patch is committed first.

-Andrea

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

https://reviews.llvm.org/D55780