Tried implementing SPF_ABS and instcombine for ABS(ABS(X))
I left ABS(-X) -> ABS(X) from TODO comment because it was not
reducing instructions count. For calculating ABS(X), it generate
instruction to negate X for select instruction.
Details
Details
Diff Detail
Diff Detail
Event Timeline
| lib/Transforms/InstCombine/InstCombineSelect.cpp | ||
|---|---|---|
| 72 | What about X < 0 ? -X : X;? | |
Comment Actions
updated code to handle following patterns [Thanks for pointing out]
// (X > 0) ? X : -X // (X < 0) ? -X : X // (X > 1) ? -X : X
Comment Actions
corrected one typo in comments.
I have written
(x > 1)? -x : x
whereas I meant
(x < 1)? -x : x
Comment Actions
One potential bug, and a couple of small comments, but otherwise looks fine.
(Note: I don't have commit rights so this is not an approval to commit.)
| lib/Transforms/InstCombine/InstCombineSelect.cpp | ||
|---|---|---|
| 34–41 | Personally, I find the code with these extracted much harder to read. I would prefer you not extract these, but don't have a strong preference. i.e. feel free to ignore | |
| 58–59 | An unsigned comparison against -1 doesn't seem right here. -1 is going to be the all ones pattern; nothing will be greater than that. I think what you want is: if minus one: return SPF_ABS fallthrough case ICmpInst::ICMP_UGT: if zero: return SFP_ABS break | |
| 62 | You could add cases for "<=" or ">=" here as well right? You might also add a comment why you can't recognize Neg(Abs(X)) patterns. (i.e. the most negative int problem) Glancing at your code it seems like an obvious extension and isn't immediate obvious why you can't. | |
Comment Actions
Thanks Philip. I appreciate your effort.
I do have one request though. As you are also new to llvm, it will be great if you can avoid
using strong words until you have some familiarity with the module in context.
| lib/Transforms/InstCombine/InstCombineSelect.cpp | ||
|---|---|---|
| 34–41 | I think now they are more readable. I have seen similar code in many places in inst combine and llvm in general. But I too do not have any strong preference and will change it if others agree with you. | |
| 58–59 | I agree that it is written in the way you have suggested. But I don't think this is bug in current code. It will simply never reach to MINUS_ONE check in UGT case. I will be happy to change it and ULT (X, 0) case too. Thanks for pointing it out. | |
| 62 | Due to other transform, we will never get <= or >= case here. These cases will always transform to < or > so we don't need to check them here. My understanding may be wrong. Let's other comment on that. I have never mentioned I can't. For NEG(ABS(X)), before reaching to this instruction, ABS(X) will be analyzed and my patch is able to recognize it as ABS. I don't think NEG(ABS(X)) is valid pattern here in current context. This patch even transform ABS(-ABS(X)) to ABS(X). I hope this makes it clear to you. | |
Comment Actions
Updated patch to handle both ABS(X) and NEG(ABS(X)) and tests for all possible combinations or ABS(ABS) and NABS(NABS)
I have removed check for unsigned predicates as ABS of unsigned values are value them selves and must already handled before.
Thanks to Philip and sorry as I did not get your comment right previously.
Comment Actions
With the last round of changes, this looks fine to me. I'm unclear whether this is covering all useful patterns (i.e. why don't we need GTE, or LTE pattens?), but what's here seems correct and useful.
Reminder: You will need to find another reviewer. I do not have commit rights and can not approve your change.
Comment Actions
Thanks for review, Philip.
InstCombine code changes >= and <= to corrosponding > and < before this code get hit.
You can try with this code with clang build in debug mode or Release+Assert
> cat test.cpp
int test(int x) {
int y = (x >= 0) ? x : -x;
int z = (y >= 0) ? y : -y;
return z;
}
> clang -O0 -S -emit-llvm test.cpp
> opt -mem2reg -instcombine -debug test.llLet me know if you have any doubts.
Comment Actions
The updated patch LGTM. Thanks!
We don't have to match patterns for sle and uge as instcombine canonicalizes them into slt and ugt. That makes code like this simpler.
Personally, I find the code with these extracted much harder to read. I would prefer you not extract these, but don't have a strong preference. i.e. feel free to ignore