[PATCH] Switch to Select optimisations
Hans Wennborg
hans at chromium.org
Mon Sep 29 13:19:51 PDT 2014
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3457
@@ +3456,3 @@
+ // Support only the case where the switch branches directly or through an
+ // intermediate unconditional block to the common destination.
+ if (CaseSuccessor != CommonDest &&
----------------
Why is this check needed? Doesn't GetCaseResults already check that the case leads to the common destination unconditionally?
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3466
@@ +3465,3 @@
+ auto ResultFound =
+ std::find_if(UniqueResults.begin(), UniqueResults.end(),
+ [&Result](const ResultVectorTy::value_type &t) {
----------------
If I understand correctly, UniqueResults is a mapping from result to vector of cases. Could we use a DenseMap instead and save some code here?
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3478
@@ +3477,3 @@
+
+ // Append the result from this case to the list and find the PHI
+ if (PHI == nullptr)
----------------
I guess the appending happened above, so maybe just "find the PHI"?
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3484
@@ +3483,3 @@
+ }
+ // Find the default result value
+ SmallVector<std::pair<PHINode *, Constant *>, 1> DefaultResults;
----------------
nit: a period at the end of comments is preferred.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3490
@@ +3489,3 @@
+ // If the default value is not found abort unless the default destination
+ // is unreachable.
+ DefaultResult =
----------------
This doesn't seem to take into account that there could be other instructions in the default destination before the unreachable terminator.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3497
@@ +3496,3 @@
+ (DefaultDest != CommonDest &&
+ !(BI && BI->isUnconditional() && BI->getSuccessor(0) == CommonDest)))
+ return false;
----------------
again, doesn't GetCaseResults already check that the case leads to CommonDest?
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3537
@@ +3536,3 @@
+// switch (a) {
+// case 10: %0 = icmp eq i32 %a, 1
+// return 10; %1 = select i1 %0, i32 10, i32 4
----------------
i assume the icmp should be with 10 here
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3550
@@ +3549,3 @@
+ // select or a two-way select if default is possible.
+ if (ResultVector[0].second.size() == 1 &&
+ ResultVector[1].second.size() == 1) {
----------------
i would add an assert about ResultVector.size()
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3599
@@ +3598,3 @@
+ const bool SecondIsBigger =
+ MaxCaseFirst->getValue().slt(MinCaseSecond->getValue());
+ // If the case statements ranges do not overlap use a comparison to
----------------
Could these comparisons be done with sgt instead? I'd find that easier to read for code that checks whether a is bigger than b.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3617
@@ +3616,3 @@
+
+// ConvertBitPatternSwitch - Helper function that checks if it is possible to
+// transform a switch with cases that are groupable by some bit pattern into
----------------
I haven't had time to look at this one yet.
Do you think the bit pattern variant could be broken out to a separate patch?
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3698
@@ +3697,3 @@
+
+/// SwitchToSelect - If the switch is only used to initialize one or more
+/// phi nodes in a common successor block with only two different
----------------
I don't think the "or more" case is supported, though?
http://reviews.llvm.org/D5525
More information about the llvm-commits
mailing list