Page MenuHomePhabricator

Added instcombine for 'ABS(ABS(X)) -> ABS(X)'
ClosedPublic

Authored by dinesh.d on May 7 2014, 12:53 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

dinesh.d updated this revision to Diff 9187.May 7 2014, 12:53 PM
dinesh.d retitled this revision from to Added instcombine for 'ABS(ABS(X)) -> ABS(X)'.
dinesh.d updated this object.
dinesh.d edited the test plan for this revision. (Show Details)
dinesh.d added reviewers: nadav, chandlerc, rafael.
dinesh.d added a subscriber: Unknown Object (MLST).
dinesh.d updated this revision to Diff 9193.May 7 2014, 3:08 PM

Replace isa<> and cast with dyn_cast.

bkramer added inline comments.May 14 2014, 8:52 AM
lib/Transforms/InstCombine/InstCombineSelect.cpp
73 ↗(On Diff #9193)

What about X < 0 ? -X : X;?

dinesh.d updated this revision to Diff 9432.May 15 2014, 4:04 AM

updated code to handle following patterns [Thanks for pointing out]

// (X > 0) ? X : -X
// (X < 0) ? -X : X
// (X > 1) ? -X : X
dinesh.d updated this revision to Diff 9461.May 15 2014, 11:50 PM

corrected one typo in comments.
I have written

(x > 1)? -x : x

whereas I meant

(x < 1)? -x : x
reames added a subscriber: reames.May 28 2014, 10:25 AM

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 ↗(On Diff #9461)

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

83 ↗(On Diff #9461)

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:
case ICmpInst::ICMP_SGT:

if minus one: return SPF_ABS
fallthrough

case ICmpInst::ICMP_UGT:

if zero: return SFP_ABS
break
93 ↗(On Diff #9461)

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.

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 ↗(On Diff #9461)

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.

83 ↗(On Diff #9461)

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.

93 ↗(On Diff #9461)

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.

dinesh.d updated this revision to Diff 9955.May 30 2014, 2:11 AM

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.

reames added a comment.Jun 4 2014, 2:43 PM

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.

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

Let me know if you have any doubts.

bkramer accepted this revision.Jun 5 2014, 11:43 AM
bkramer edited edge metadata.

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.

This revision is now accepted and ready to land.Jun 5 2014, 11:43 AM
dinesh.d closed this revision.Jun 6 2014, 12:02 AM
dinesh.d updated this revision to Diff 10164.

Closed by commit rL210312 (authored by dinesh).