[PATCH] Switch to Select optimisations
Hans Wennborg
hans at chromium.org
Wed Oct 1 17:45:37 PDT 2014
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:79
@@ +78,3 @@
+ typedef SmallVector<std::pair<Constant *, SmallVector<ConstantInt *, 4>>, 2>
+ SwitchCaseResultVectorTy;
+ typedef SmallVector<std::pair<PHINode *, Constant *>, 4> SwitchCaseResultsTy;
----------------
Thanks! These names are better. A comment explaining what the components of the pair represent would also be great.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3435
@@ -3430,1 +3434,3 @@
+// MapCaseToResult - This is an helper function used to map a case value
+// of a switch to a the result that is produced when the case is selected.
----------------
How about: "add CaseVal to the list of cases that generate Result".
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3473
@@ +3472,3 @@
+
+ // Check the PHI and be sure it to be consistent.
+ if (PHI == nullptr)
----------------
nit: "be sure it to be" ?
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3474
@@ +3473,3 @@
+ // Check the PHI and be sure it to be consistent.
+ if (PHI == nullptr)
+ PHI = Results[0].first;
----------------
It does occur in LLVM code, but I think the more common style is to not do explicit comparisons against nullptr, but rather do
if (!PHI) {
This applies throughout the patch and I think will simplify some lines.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3489
@@ +3488,3 @@
+ if ((DefaultResult == nullptr &&
+ !isa<UnreachableInst>(DefaultDest->getFirstNonPHIOrDbg())))
+ return false;
----------------
Just check for nullptr with !DefaultResult, and drop the extra parentheses.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3501
@@ +3500,3 @@
+// KnownBitsCase are instead the bits known to be one or zero for all the case
+// values contained in the vector of cases passed as input.
+static void AnalyzeSwitchCases(const SmallVectorImpl<ConstantInt *> &Cases,
----------------
Thanks! I think the expanded comment and new variable names helps the readability of this code a lot.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3510
@@ +3509,3 @@
+ MaxCase = Cases[0];
+ // Compute the number of patterns covered by the cases of each result,
+ // the minimum and maximum cases and the zeroes and ones for this case group.
----------------
I would break this comment up into three comments and move them down to where the respective computation occurs in the for loop, i.e.
// Compute number of cases matching the known bits of the condition.
(code)
// Update known bits of case values.
(code)
// Compute min and max case value.
(code)
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3542
@@ +3541,3 @@
+static Value *
+ConvertTwoCaseSwitch(SwitchCaseResultVectorTy &ResultVector,
+ Constant *DefaultResult, Value *Condition,
----------------
Make ResultVector const-ref.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3556
@@ +3555,3 @@
+ Value *SelectValue = ResultVector[1].first;
+ if (DefaultCanTrigger) {
+ Value *const ValueCompare =
----------------
Actually, I'd just do
if (DefaultResult)
here.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3590
@@ +3589,3 @@
+static Value *
+ConvertRangeSwitch(SwitchCaseResultVectorTy &ResultVector,
+ Value *Condition,
----------------
Make ResultVector const-ref.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3634
@@ +3633,3 @@
+// }
+static Value *ConvertBitPatternSwitch(SwitchCaseResultVectorTy &ResultVector,
+ Value *Condition,
----------------
Make ResultVector const-ref.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3664
@@ +3663,3 @@
+ "switch.selectcmp");
+ } else if (SecondResultGroupSelectBits != 0) {
+ ConstantInt *const MaskValue =
----------------
the outer if-statement has already checked that either FirstResultGroupSelectBits or Second...Bits are non-zero, so this could just be an else branch, possibly with an assert.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3695
@@ +3694,3 @@
+
+ if (Succ == SI->getDefaultDest())
+ continue;
----------------
Can you explain (ideally by adding a comment) why we're not removing this successor?
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3710
@@ +3709,3 @@
+ BasicBlock *CommonDest = nullptr;
+ Type *ResultType;
+ Constant *DefaultResult;
----------------
I would not declare this until it's first used, which seems to be after the InitializeUniqueCases call below.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3718
@@ +3717,3 @@
+ return false;
+ const unsigned NumOfResults = UniqueResults.size();
+ // Selects choose between maximum two values.
----------------
Since this is only used in the if statement below, I'm not sure it's worth a separate variable. I'd just check UniqueResults.size() in the if statement directly.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3727
@@ +3726,3 @@
+
+ APInt KnownZeroCond(BitWidth, 0), KnownOneCond(BitWidth, 0);
+ // Compute known bits of the Condition of the Switch statement.
----------------
I'd s/Zero/Zeroes/ and s/One/Ones/ in the second variable name.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3746
@@ +3745,3 @@
+ // still transform it into a select
+ bool DefaultCanTrigger;
+ APInt FirstResultGroupZeroBits = APInt::getAllOnesValue(BitWidth);
----------------
No point declaring the variable until it's used.
================
Comment at: test/Transforms/SimplifyCFG/UnreachableEliminate.ll:51
@@ -50,2 +50,3 @@
define i32 @test4(i32 %v) {
+; CHECK-LABEL: @test4(
; CHECK: entry:
----------------
Doesn't this change the test to not check the problem in the original bug? If you change one of the results that the switch go to to not be constant, that should defeat the switch-to-select transform.
================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-bitpattern.ll:3
@@ +2,3 @@
+
+; int foo3_with_def(int a) {
+; switch (a) {
----------------
might as well spell out "def" as "default" here and below to simplify for the reader
================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-bitpattern.ll:18
@@ +17,3 @@
+;
+; int foo3_without_def(int a) {
+; a &= 0x6;
----------------
i'd put this comment down by the llvm function it represents
================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-bitpattern.ll:34
@@ +33,3 @@
+
+define i32 @foo3_with_def(i32 %a) #0 {
+; CHECK-LABEL: @foo3_with_def
----------------
I'd drop the #0. Also a comment saying what's being tested, i.e. that this switch can't be transformed to select because it has a default.
================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-bitpattern.ll:65
@@ +64,3 @@
+
+define i32 @foo3_without_def(i32 %a) #0 {
+; CHECK-LABEL: @foo3_without_def
----------------
i'd drop the #0
================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-bitpattern.ll:67
@@ +66,3 @@
+; CHECK-LABEL: @foo3_without_def
+; CHECK: select
+entry:
----------------
i think we should check more explicitly what's in the select.
================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-bitpattern.ll:95
@@ +94,2 @@
+ ret i32 %retval.0
+}
----------------
would be nice to have tests for both the default with unreachable instruction, and the clever case where we know enough bits to figure out the default case can never happen.
================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-range.ll:3
@@ +2,3 @@
+
+define i32 @foo2_with_def(i32 %a) #0 {
+; CHECK-LABEL: @foo2_with_def
----------------
again i'd drop the #0 here and below
================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-range.ll:5
@@ +4,3 @@
+; CHECK-LABEL: @foo2_with_def
+; CHECK-NOT: select
+entry:
----------------
a comment about what the test is intended to test, i.e. why this can't become a select, would be good.
================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-range.ll:36
@@ +35,3 @@
+; CHECK-LABEL: @foo2_without_def
+; CHECK: select
+entry:
----------------
we should check the select explicitly
================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-two-case.ll:13
@@ +12,3 @@
+;
+; int foo1_without_def(int a) {
+; switch(a) {
----------------
again, i'd put the comment by the llvm function definition
================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-two-case.ll:26
@@ +25,3 @@
+; CHECK: icmp eq i32
+; CHECK-NEXT: select i1
+; CHECK-NEXT: icmp eq i32
----------------
we should check exactly what's being selected and compared here
http://reviews.llvm.org/D5525
More information about the llvm-commits
mailing list