Page MenuHomePhabricator

Unsigned saturation subtraction canonicalization
ClosedPublic

Authored by yulia_koval on Sep 6 2017, 4:56 AM.

Details

Summary

Hi, this is follow-up patch for https://reviews.llvm.org/D25987 and http://lists.llvm.org/pipermail/llvm-dev/2017-May/113113.html.
The first patch adds tests for the new canonical psubus pattern.
The second patch adds backend support for this pattern.

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).

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 https://reviews.llvm.org/D25987).

Diff Detail

Repository
rL LLVM

Event Timeline

yulia_koval created this revision.
yulia_koval created this object with visibility "No One".

This is the second patch, containing backend-code for new pattern.

yulia_koval changed the visibility from "No One" to "Public (No Login Required)".Sep 6 2017, 4:58 AM
yulia_koval edited the summary of this revision. (Show Details)Sep 6 2017, 5:08 AM
zvi edited edge metadata.Sep 6 2017, 9:44 AM

Please update the Summary with more details about the combine that was added.

test/CodeGen/X86/psubus.ll
13 ↗(On Diff #113990)

Can you please run the update_llc_tests tool without this patch and commit changes to remove the "End function" diffs from the patch?

1102 ↗(On Diff #113990)

Please rebase the patch after running update_llc_tests so that all above changes will go away.

In D37510#862264, @zvi wrote:

Please update the Summary with more details about the combine that was added.

Here is the commit made by update_test run without the patch:
https://reviews.llvm.org/D37523

tests, rebased

Moved second patch(backend part) to extra revision for convenience https://reviews.llvm.org/D37534.

yulia_koval edited the summary of this revision. (Show Details)Sep 6 2017, 1:52 PM
spatel edited edge metadata.Sep 7 2017, 7:01 AM

Is this patch intended to just add the baseline tests, or is there an associated code change that didn't get uploaded?

DavidKreitzer added inline comments.Sep 7 2017, 2:10 PM
test/CodeGen/X86/psubus.ll
1049 ↗(On Diff #114064)

Should we add AVX-512 support while we are at it (both here and in the functional patch, https://reviews.llvm.org/D37534)?

1475 ↗(On Diff #114064)

You added a 'min' version of this test but not the others. Was that intentional or did you mean to add 'min' versions of all of them?

Is this patch intended to just add the baseline tests, or is there an associated code change that didn't get uploaded?

This patch just adds tests, there is a followup patch, which adds backend changes for generating psubus in these cases:
https://reviews.llvm.org/D37534.

yulia_koval added inline comments.Sep 7 2017, 3:38 PM
test/CodeGen/X86/psubus.ll
1475 ↗(On Diff #114064)

I just decided, that I need to cover each code path once. Should I add min versions of all other tests, I added?

DavidKreitzer added inline comments.Sep 8 2017, 5:57 AM
test/CodeGen/X86/psubus.ll
1475 ↗(On Diff #114064)

No, that's not necessary. Thanks for the explanation.

zvi added a comment.Sep 11 2017, 6:48 AM

Looks ready apart from the missing AVX512 testing Dave suggested.

test/CodeGen/X86/psubus.ll
1049 ↗(On Diff #114064)

There was no 512-bit psubus instruction added to AVX512, so IMO it is sufficient to add a RUN: command for a AVX512 and ensure we are at least as good as AVX2.

zvi added inline comments.Sep 11 2017, 12:07 PM
test/CodeGen/X86/psubus.ll
1049 ↗(On Diff #114064)

I take that back - 512-bit instructions are available. Sorry.

Added 512bit support.

zvi added inline comments.Sep 27 2017, 3:22 AM
test/CodeGen/X86/psubus.ll
7 ↗(On Diff #116757)
  1. +avx512vl
  2. Since there is only one RUN: line for AVX512 the "-check-prefix=AVX512BWVL" is redundant
zvi accepted this revision.Sep 27 2017, 5:42 AM

LGTM

This revision is now accepted and ready to land.Sep 27 2017, 5:42 AM
In D37510#882071, @zvi wrote:

LGTM

Thanks, could you please commit it for me?

This revision was automatically updated to reflect the committed changes.