Page MenuHomePhabricator

Switch to Select optimisations - Two Case switch to select
ClosedPublic

Authored by kariddi on Oct 5 2014, 11:24 AM.

Details

Reviewers
hans
Summary

This optimization is derived from the optimisations in http://reviews.llvm.org/D5525

The optimisations in D5525 are of three kind. This patch contains only one kind (the Convert Two case switch to select), for review.

Checkout D5525 initial comment for a full description of all the optimisations.

Diff Detail

Event Timeline

kariddi updated this revision to Diff 14435.Oct 5 2014, 11:24 AM
kariddi retitled this revision from to Switch to Select optimisations - Two Case switch to select.
kariddi updated this object.
kariddi edited the test plan for this revision. (Show Details)
kariddi added a reviewer: hans.
kariddi set the repository for this revision to rL LLVM.
kariddi added subscribers: Unknown Object (MLST), hans.
hans edited edge metadata.Oct 6 2014, 11:00 AM

Thanks for breaking this out to a separate patch! It makes it much easier to review.

I think this is ready to be committed with the comments below addressed. Do you have commit access, or would you like me to land it for you?

lib/Transforms/Utils/SimplifyCFG.cpp
3453

nit: there is no need to repeat the function name in the comment. This goes for the other functions too.

And instead of "This is an helper function used to..." I'd just put "Helper function that adds CaseVal to the list of cases that generate Result."

3602

The lines from "const unsigned BitWidth" to "computeKnownBits(...)" look like leftovers from the other patch.

test/Transforms/SimplifyCFG/switch-to-select-two-case.ll
16

Please check the full icmp instruction, i.e. capture the name and what is being compared.

52

Please check the full icmp instruction.

I do have commit access :)
Let me know if you think it is better removing function names from the comments or not considering most of the functions have them, and after addressing this last thing I will commit it.

Thanks again!

lib/Transforms/Utils/SimplifyCFG.cpp
3453

I put the name in the comment, because most of the other functions in the file follow that style.
I thought it was a good idea to follow the general style in the file.

As far as "This is ..." is concerned I think you are right, that piece is redundant.

3602

Whoops :D

test/Transforms/SimplifyCFG/switch-to-select-two-case.ll
16

Done

kariddi added inline comments.Oct 6 2014, 8:11 PM
test/Transforms/SimplifyCFG/switch-to-select-two-case.ll
52

done

kariddi updated this revision to Diff 14488.Oct 6 2014, 8:12 PM
kariddi edited edge metadata.

Latest comments taken care of.

hans accepted this revision.Oct 7 2014, 9:21 AM
hans edited edge metadata.

You're right, repeating the function name in the comment seems to be the dominating style in this file. Let's not worry about it, then.

This revision is now accepted and ready to land.Oct 7 2014, 9:21 AM
kariddi closed this revision.Oct 7 2014, 11:35 AM

Landed r219223