Page MenuHomePhabricator

[X86] Unsigned saturation subtraction canonicalization [the backend part]

Authored by yulia_koval on Sep 6 2017, 1:46 PM.



This is the backend part of unsigned saturation canonicalization patch.

The patch transforms canonical version of unsigned saturation, which is sub(max(a,b),a) or sub(a,min(a,b)) to special psubus insturuction on targets, which support it(8bit and 16bit uints).
umax(a,b) - b -> subus(a,b)
a - umin(a,b) -> subus(a,b)

There is also extra case handled, when right part of sub is 32 bit and can be truncated, using UMIN(this transformation was discussed in

The example of special case code:

void foo(unsigned short *p, int max, int n) {

  int i;
  unsigned m;
  for (i = 0; i < n; i++) {
    m = *--p;
    *p = (unsigned short)(m >= max ? m-max : 0);

Max in this example is truncated to max_short value, if it is greater than m, or just truncated to 16 bit, if it is not. It is vaid transformation, because if max > max_short, result of the expression will be zero.

Here is the table of types, I try to support, special case items are bold:


Diff Detail

Event Timeline

yulia_koval created this revision.Sep 6 2017, 1:46 PM
zvi set the repository for this revision to rL LLVM.
zvi added a subscriber: llvm-commits.
zvi edited edge metadata.Sep 11 2017, 7:19 AM

Please add some details about the combine in the Summary. It's better to put a description in addition to the links to make it easier to read the commit log.
Maybe something like:
umax(a,b) - b -> subus(a,b)
a - umin(a,b) -> subus(a,b)

And better add an X86 label to the title.


Would a better place for this comment be right after the '// Try to find ... ' comment?


Please drop braces for if/else with a single statement


Can we generalize by using DAG.computeKnownBits to tells us that the upper bits are zeroed?

yulia_koval edited the summary of this revision. (Show Details)

Added 512bit support and fixed comments

yulia_koval retitled this revision from Unsigned saturation subtraction canonicalization [the backend part] to [X86] Unsigned saturation subtraction canonicalization [the backend part].Sep 26 2017, 10:27 PM
yulia_koval edited the summary of this revision. (Show Details)
zvi added a comment.Sep 27 2017, 2:43 AM

Please add the avx512vl feature to the AVX512 test (in the parent patch) and rebase this patch. It will drastically improve some of the cases :).


Please drop the braces

zvi added inline comments.Sep 28 2017, 7:06 AM
2383 ↗(On Diff #116797)

Looks like in this case SSE4.1, AVX1 and AVX2 regressed. Can you please take a look at it?

Fixed problem on min case.

yulia_koval marked an inline comment as done.Oct 2 2017, 12:58 AM
zvi accepted this revision.Oct 9 2017, 12:12 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Oct 9 2017, 12:12 AM

Thanks! Could you please commit it for me?

This revision was automatically updated to reflect the committed changes.
spatel added inline comments.Oct 11 2017, 10:35 AM
35928–35930 ↗(On Diff #118259)

Sorry I didn't have a chance to look at this closely before commit, but why are we using isEqualTo() here? This pattern only applies to integer types, so we could use simple equality checks instead?

Thanks, fixed your comment in additional patch below.

spatel edited edge metadata.Oct 12 2017, 9:23 AM

Thanks, fixed your comment in additional patch below.

Thank you - committed with rL315589.