This is an archive of the discontinued LLVM Phabricator instance.

Added instcombine for 'MIN(MIN(A, 27), 93)' and 'MAX(MAX(A, 93), 27)'
ClosedPublic

Authored by dinesh.d on May 6 2014, 2:00 PM.

Details

Summary

Added instcombine for 'MIN(MIN(A, 27), 93)' and 'MAX(MAX(A, 93), 27)'

MIN(MIN(A, 23), 97) -> MIN(A, 23)
MAX(MAX(A, 97), 23) -> MAX(A, 97)

Diff Detail

Event Timeline

dinesh.d updated this revision to Diff 9129.May 6 2014, 2:00 PM
dinesh.d retitled this revision from to Added instcombine for 'MIN(MIN(A, 27), 93)' and 'MAX(MAX(A, 93), 27)'.
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 9160.May 7 2014, 5:45 AM

Changed if blocks to switch block and updated test cases

rafael edited edge metadata.May 7 2014, 12:24 PM

Thanks.

lib/Transforms/InstCombine/InstCombineSelect.cpp
69

Why not implement it in here? In any case, this patch is no changing this, right? Maybe just leave the FIXME as is?

662

please clang-format the patch.

668

use a dyn_cast instead of an isa above. That way you don't need to cast in here.

671

I don't thin get can fail.

Creating a ConstExpr just to evaluate it seems wasteful. Can you compare the constants directly?

Do we a/ready handle cases like

MIN(MIN(A, 97), 23)?

If not, can you leave a FIXME in place for it.

Thanks for review.

lib/Transforms/InstCombine/InstCombineSelect.cpp
69

I tried doing it but it has already been handled in visitSelectInstWithICmp()
If we want it here, I can refactor it and put that code here.

662

clang-format was formatting code lines in case statement in new line.
I have seen similar pattern in other places so tried to keep it similar.
I did run clang-format in update patch.

668

updated

671

I missed 'SI.getType()->isIntegerTy()' call before calling this function.
So wasn't sure that I can use ConstantInt and compare them. Update
in next patch.

Added TODO for MIN(MIN(A, 97), 23). I will try to fix it and send patch
for review soon.

dinesh.d updated this revision to Diff 9192.May 7 2014, 2:34 PM
dinesh.d edited edge metadata.

Updated as per comments.

rafael accepted this revision.May 14 2014, 12:45 PM
rafael edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 14 2014, 12:45 PM
dinesh.d closed this revision.May 14 2014, 11:22 PM

Submitted as r208849.

Thanks for review and support.