Page MenuHomePhabricator

[ARM] Lower (select_cc k k (select_cc ~k ~k x)) into (SSAT l_k x)
ClosedPublic

Authored by pbarrio on Jun 15 2016, 4:17 AM.

Details

Summary

SSAT saturates an integer, making sure that its value lies within
an interval [-k, k]. Since the constant is given to SSAT as the
number of bytes set to one, k + 1 must be a power of 2, otherwise
the optimization is not possible. Also, the select_cc must use <
and > respectively so that they define an interval.

Diff Detail

Repository
rL LLVM

Event Timeline

pbarrio updated this revision to Diff 60813.Jun 15 2016, 4:17 AM
pbarrio retitled this revision from to [ARM] Lower (select_cc k k (select_cc ~k ~k x)) into (SSAT l_k x).
pbarrio updated this object.
pbarrio added reviewers: mcrosier, jmolloy.
pbarrio added a subscriber: llvm-commits.

Hello mcrosier, jmolloy, rengolin, would you have a few minutes to review this patch?

Thanks

Sorry it took so long, I was put of by some of the comments and tests and kept getting distracted.

Some comments to start... :)

cheers,
--renato

lib/Target/ARM/ARMISelLowering.cpp
3792 ↗(On Diff #60813)

This comment is misleading (and it took me a while to figure out :).

This is not lowering (select o select), but (select o (lower o (select o upper), which is a completely different case. I think the comment above on the possibilities should actually be here.

3850 ↗(On Diff #60813)

This is a slightly deeper nesting that we normally like. Can you refactor this into a new function and use early returns?

test/CodeGen/ARM/ssat.ll
12 ↗(On Diff #60813)

I'd add the base test for i8 and i16, just to make sure we're getting it right with all the trivially extensible types.

29 ↗(On Diff #60813)

cond is a bad name and also gave me some time to think about... Maybe max and min would be better names for those.

It might sound picky to comment on those names, but reading tests is the best way to understand what the intention of the code is, and so act much more as documentation than comment lines or commit messages. :)

199 ↗(On Diff #60813)

Try to give more meaningful names of why not. Is the range wrong? Are they not powers of two? Are the conditions reversed? etc. At the very least, add a comment to that effect.

201 ↗(On Diff #60813)

The NOT tests are better with just "ssat" not with the #24, since you could still emit another sized SSAT and it will still be wrong.

pbarrio updated this revision to Diff 61386.Jun 21 2016, 9:01 AM

Addressed concerns about refactoring, comments and testing

Renato, thank you for the review. I have refactored the logic into a separate
function and used early returns as suggested. I have also modified the comments
so that they are clearer.

I have extensively cleaned up the tests and added some more. This time, the naming
is much better because it's done by me :) The previous tests were directly taken
from compiling a C file with use cases into IR, hence the cryptic names. Some of the
tests were also redundant because LLVM normalizes some constructs, so different C
code was transformed into the same IR. I have removed these - lesson learned.

I hope you find this version more readable, but let me know if you find room for
improvement. Thanks again!

pbarrio updated this revision to Diff 61394.Jun 21 2016, 9:48 AM

Small fix for one of the tests

pbarrio updated this revision to Diff 61396.Jun 21 2016, 10:04 AM

Same fix for another test.

Thanks Pablo, it's looking much better!

A few remaining comments...

--renato

lib/Target/ARM/ARMISelLowering.cpp
3786 ↗(On Diff #61396)

Minor nit, maybe move this line down like the other above?

3837 ↗(On Diff #61396)

Wait, this looks backwards...

Shouldn't the positive value be something like:

std::max(Val1, Val2);

but you seem to be getting the lowest of both sign extended values.

test/CodeGen/ARM/ssat.ll
183 ↗(On Diff #61396)

change all CHECK-NOT lines to:

; CHECK-NOT: ssat

as we don't want *an* ssat being generated, no matter how the test changes in the future.

pbarrio marked an inline comment as done.Jun 22 2016, 8:47 AM
pbarrio added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
3837 ↗(On Diff #61396)

I'm comparing two unsigned integers, so effectively the "negative" numbers are bigger than the positive ones because the most significant bit is always 1.

I'm doing this because getSExtValue() returns a uint64_t. I could cast Val1 and Val2 to signed and therefore do a signed integer compare. Let me know if you prefer that.

pbarrio added inline comments.Jun 22 2016, 8:51 AM
lib/Target/ARM/ARMISelLowering.cpp
3837 ↗(On Diff #61396)

I will shut up. getSExtValue returns an int64_t so there is no reason to do this the convoluted way. I will change it in the next version.

rengolin added inline comments.Jun 22 2016, 8:54 AM
lib/Target/ARM/ARMISelLowering.cpp
3837 ↗(On Diff #61396)

:D

pbarrio updated this revision to Diff 61582.Jun 22 2016, 11:55 AM
pbarrio marked 3 inline comments as done.

Second round of changes addressed. Thanks!

Also a side-question. Would it be better to use std::max(V1, V2) instead of
V1 > V2 ? V1 : V2? If so, why?

rengolin accepted this revision.Jun 22 2016, 12:26 PM
rengolin added a reviewer: rengolin.

Second round of changes addressed. Thanks!

Thanks! Looks good to me now, + using std::max.

Also a side-question. Would it be better to use std::max(V1, V2) instead of
V1 > V2 ? V1 : V2? If so, why?

Yes, because I first assumed it was it, so didn't bothered. It was only when I looked again that it didn't make sense because the condition was reversed but the values were unsigned. While it could potentially work on all cases, it was an obtuse construct, and prone to confusion.

If you had used std::min from the beginning, I'd have caught it early on, while I could have missed it with the ternary.

From a code generation point of view, std::max/min generates the exact same code as the ternary, so there's no reason to not use it.

cheers,
--renato

This revision is now accepted and ready to land.Jun 22 2016, 12:26 PM
pbarrio updated this revision to Diff 61671.Jun 23 2016, 7:02 AM
pbarrio edited edge metadata.

Using std::max() now. Near that, I also made an implicit cast explicit, just for readability.

LGTM, thanks!

This revision was automatically updated to reflect the committed changes.