[PATCH] Switch to Select optimisations

Marcello Maggioni hayarms at gmail.com
Thu Oct 2 05:23:47 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;
----------------
hans wrote:
> Thanks! These names are better. A comment explaining what the components of the pair represent would also be great.
Added

================
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. 
----------------
hans wrote:
> How about: "add CaseVal to the list of cases that generate Result".
Done!

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3473
@@ +3472,3 @@
+
+    // Check the PHI and be sure it to be consistent.
+    if (PHI == nullptr)
----------------
hans wrote:
> nit: "be sure it to be" ?
Modified :P

================
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;
----------------
hans wrote:
> 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.
I see. Sorry, I'm very used to the other way or writing it (because I somehow like it more because is more explicit) so I always forget of this rule :P

Fixed.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3489
@@ +3488,3 @@
+  if ((DefaultResult == nullptr &&
+        !isa<UnreachableInst>(DefaultDest->getFirstNonPHIOrDbg())))
+    return false;
----------------
hans wrote:
> Just check for nullptr with !DefaultResult, and drop the extra parentheses.
Done

================
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.
----------------
hans wrote:
> 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)
Done.
I also kept the common outside of the loop, so that it works like an overview and the inner comments specify where that happens.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3542
@@ +3541,3 @@
+static Value *
+ConvertTwoCaseSwitch(SwitchCaseResultVectorTy &ResultVector,
+                     Constant *DefaultResult, Value *Condition,
----------------
hans wrote:
> Make ResultVector const-ref.
Done

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3556
@@ +3555,3 @@
+    Value *SelectValue = ResultVector[1].first;
+    if (DefaultCanTrigger) {
+      Value *const ValueCompare =
----------------
hans wrote:
> Actually, I'd just do
> 
>   if (DefaultResult)
> 
> here.
Hmm, I prefer here changing DefaultCanTrigger to :

DefaultCanTrigger = DefaultResult;

and then use DefaultCanTrigger in the if, because that is used everywhere else we need to check if the default can trigger, so seems a little bit more coherent in my opinion. (Makes more clear to the reader what we want to test for)
What do you think?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3590
@@ +3589,3 @@
+static Value *
+ConvertRangeSwitch(SwitchCaseResultVectorTy &ResultVector,
+                   Value *Condition,
----------------
hans wrote:
> Make ResultVector const-ref.
Done

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3634
@@ +3633,3 @@
+// }
+static Value *ConvertBitPatternSwitch(SwitchCaseResultVectorTy &ResultVector,
+    Value *Condition,
----------------
hans wrote:
> Make ResultVector const-ref.
Done.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3664
@@ +3663,3 @@
+          "switch.selectcmp");
+    } else if (SecondResultGroupSelectBits != 0) {
+      ConstantInt *const MaskValue =
----------------
hans wrote:
> 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.
Good catch!

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3695
@@ +3694,3 @@
+
+    if (Succ == SI->getDefaultDest())
+      continue;
----------------
hans wrote:
> Can you explain (ideally by adding a comment) why we're not removing this successor?
I think this is actually a bug ... the check should be with Destination.
I also added a fix for the PHI node.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3710
@@ +3709,3 @@
+  BasicBlock *CommonDest = nullptr;
+  Type *ResultType;
+  Constant *DefaultResult;
----------------
hans wrote:
> I would not declare this until it's first used, which seems to be after the InitializeUniqueCases call below.
I think ResultType is not used anymore now.
Removed!

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3718
@@ +3717,3 @@
+    return false;
+  const unsigned NumOfResults = UniqueResults.size();
+  // Selects choose between maximum two values.
----------------
hans wrote:
> 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.
Done

================
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.
----------------
hans wrote:
> I'd s/Zero/Zeroes/ and s/One/Ones/ in the second variable name.
Done.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3746
@@ +3745,3 @@
+  // still transform it into a select
+  bool DefaultCanTrigger;
+  APInt FirstResultGroupZeroBits = APInt::getAllOnesValue(BitWidth);
----------------
hans wrote:
> No point declaring the variable until it's used.
Legacy declaration :|
Done.

================
Comment at: test/Transforms/SimplifyCFG/UnreachableEliminate.ll:51
@@ -50,2 +50,3 @@
 define i32 @test4(i32 %v) {
+; CHECK-LABEL: @test4(
 ; CHECK: entry:
----------------
hans wrote:
> 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.
Fixed!

================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-bitpattern.ll:3
@@ +2,3 @@
+
+; int foo3_with_def(int a) {
+;   switch (a) {
----------------
hans wrote:
> might as well spell out "def" as "default" here and below to simplify for the reader
Ok.

================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-bitpattern.ll:18
@@ +17,3 @@
+;
+; int foo3_without_def(int a) {
+;   a &= 0x6;
----------------
hans wrote:
> i'd put this comment down by the llvm function it represents
Good idea.

================
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
----------------
hans wrote:
> 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.
Done

================
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
----------------
hans wrote:
> i'd drop the #0
Done

================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-bitpattern.ll:67
@@ +66,3 @@
+; CHECK-LABEL: @foo3_without_def
+; CHECK: select
+entry:
----------------
hans wrote:
> i think we should check more explicitly what's in the select.
Done

================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-bitpattern.ll:95
@@ +94,2 @@
+  ret i32 %retval.0
+}
----------------
hans wrote:
> 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.
There is a problem actually with that.

If the default block is explicitly unreachable then the Unreachable case elimination (an optimization run earlier) substitutes the default with one of the cases.
This might move one of the case group results from the UniqueResult vector to the DefaultResult.
The UniqueResult vector reduces its size from 2 to 1 and the optimization bails out.

Actually there could be a way maybe to improve it to cover also this case ...
In the case we only have one result in UniqueResult then the SelectBitmask is just the KnownOnes for that group.
If we can determine that the cases in the result group are all and the only possible case values for the switch condition that have Ones in that position then we can use the KnownOnes as the bit mask.

So , in this example:
int foo3_explicit_without_default(int a) {
  a &= 0x6;
  switch (a) {
    case 0:
      return 1;
    case 2:
      return 2;
    case 4:
      return 1;
    case 6:
      return 2;
  }
  __builtin_unreachable();
}

We have two groups, one generating the value "2" and one generating the value "1". The default is unreachable.

after optimization becomes:

define i32 @foo3_explicit_without_default(i32 %a) nounwind readnone ssp uwtable {
  %1 = and i32 %a, 6
  switch i32 %1, label %4 [
    i32 6, label %3
    i32 2, label %2
  ]

; <label>:2                                       ; preds = %0
  br label %4

; <label>:3                                       ; preds = %0
  br label %4

; <label>:4                                       ; preds = %0, %3, %2
  %.0 = phi i32 [ 2, %3 ], [ 2, %2 ], [ 1, %0 ]
  ret i32 %.0
}

Which is not optimizable, because the group generating "1" has been moved to the default.
But , because:
- KnownOnes for the cases generating "2" is 0x4, and
- the possible bit patterns for the switch condition are only 0x0, 0x2, 0x4, 0x6 and
- The cases in the group generating "2" are all and the only valid cases that match the KnownOnes mask for "2".
Then that mask can be used to differentiate between the default and the group generating "2".

Should I modify the algorithm to match this case in this patch? Or is it better to do it separately? (It would involve changing everything to support UniqueResults.size() < 2).

================
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
----------------
hans wrote:
> again i'd drop the #0 here and below
Done

================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-range.ll:5
@@ +4,3 @@
+; CHECK-LABEL: @foo2_with_def
+; CHECK-NOT: select
+entry:
----------------
hans wrote:
> a comment about what the test is intended to test, i.e. why this can't become a select, would be good.
Done

================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-range.ll:36
@@ +35,3 @@
+; CHECK-LABEL: @foo2_without_def
+; CHECK: select
+entry:
----------------
hans wrote:
> we should check the select explicitly
Done

================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-two-case.ll:13
@@ +12,3 @@
+;
+; int foo1_without_def(int a) {
+;   switch(a) {
----------------
hans wrote:
> again, i'd put the comment by the llvm function definition
Done

================
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
----------------
hans wrote:
> we should check exactly what's being selected and compared here
Done

http://reviews.llvm.org/D5525






More information about the llvm-commits mailing list