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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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!
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. |
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. |
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. |
lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
3837 ↗ | (On Diff #61396) | :D |
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?
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
Using std::max() now. Near that, I also made an implicit cast explicit, just for readability.