This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Selects SSAT/USAT from LLVM IR of min/max patterns
ClosedPublic

Authored by MeeraN on Sep 9 2020, 6:51 AM.

Details

Summary

LLVM will canonicalize conditional selectors to a different pattern than the old code that was used.
This is updating the function to match the new expected patterns and select SSAT or USAT when successful.
Tests have also been updated to use the new patterns.

Diff Detail

Event Timeline

MeeraN created this revision.Sep 9 2020, 6:51 AM
MeeraN requested review of this revision.Sep 9 2020, 6:51 AM

It's probably worth adding something to the commit message about how llvm will canonicalize to different patterns than the old code would select from, and that this is updating it to the new expected form.

llvm/lib/Target/ARM/ARMISelLowering.cpp
5002

It looks like this isn't used any more. If not, we can remove it. Unfortunately it appears that isLowerSaturate is still used.

5003

Can you add something to this comment about llvm canonicalizing to min(max(..)) or max(min(..))

5021–5022

Can you also change usat to Usat or USat or something like it.

5094–5095

Can you move the comment to its own line, to make the formatting nicer.

You can also possibly reverse the if conditions above to return early and reduce the need for indenting.

MeeraN updated this revision to Diff 290946.Sep 10 2020, 5:08 AM
MeeraN edited the summary of this revision. (Show Details)

Added explanation about LLVM canonicalizing to min/max patterns to both the comment before the function and commit message. Tidied up formatting by moving a comment, reducing indention where possible and removing unused function (isUpperSaturate).

dmgreen accepted this revision.Sep 10 2020, 1:08 PM

Nice one. LGTM

This revision is now accepted and ready to land.Sep 10 2020, 1:08 PM

Headsup: This broke a number of tests for me. Looking closer into where it changed things erroneously...

The main difference in code, in the function that show the error in one of the testcases that broke, looks like this:

-       movs    r1, #9
-       cmp     r0, #9
-       it      lt
-       movlt   r1, r0
-       bic.w   r0, r1, r1, asr #31
+       usat    r0, #1, r0

The file that shows the error can be built from https://martin.st/temp/eval.c, built as clang -target armv7-w64-mingw32 -c -O2 eval.c.

Headsup: This broke a number of tests for me. Looking closer into where it changed things erroneously...

It seems we made a small mistake in one of the if conditions, will fix it now. Thanks for letting us know

Headsup: This broke a number of tests for me. Looking closer into where it changed things erroneously...

It seems we made a small mistake in one of the if conditions, will fix it now. Thanks for letting us know

Thanks for the fix; my testset runs fine now again.

srj added a subscriber: srj.Sep 15 2020, 9:12 AM

It seems we made a small mistake in one of the if conditions, will fix it now. Thanks for letting us know

What commit/review has the fixes? This is affecting Halide as well and we'd like to test the fix.

In D87379#2274531, @srj wrote:

It seems we made a small mistake in one of the if conditions, will fix it now. Thanks for letting us know

What commit/review has the fixes? This is affecting Halide as well and we'd like to test the fix.

The fix was in https://github.com/llvm/llvm-project/commit/1119bf95be94950da602b268dc96dbb2110cbe15 / rG1119bf95be94950da602b268dc96dbb2110cbe15