[hydrogen] optimize switch with string clauses

src/hydrogen.cc

Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 0b843bd45b32eb6d57c1efa86ee784c59290c817..1c7bdbb2a880122b33a0e4fc11836d8b0de9e1a7 100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -2690,38 +2690,65 @@ void HGraphBuilder::VisitSwitchStatement(SwitchStatement* stmt) {
   HValue* tag_value = Pop();
   HBasicBlock* first_test_block = current_block();
 
+  enum {

[Vyacheslav Egorov (Chromium), 2011/10/24 10:56:26]

We don't use anonymous enums

+    none, smi, string

[Vyacheslav Egorov (Chromium), 2011/10/24 10:56:26]

use either 

kSmiSwitch

or 

SMI_SWITCH

naming for enum-members.

+  } clause_type = none;
+
   // 1. Build all the tests, with dangling true branches.  Unconditionally
   // deoptimize if we encounter a non-smi comparison.
   for (int i = 0; i < clause_count; ++i) {
     CaseClause* clause = clauses->at(i);
     if (clause->is_default()) continue;
-    if (!clause->label()->IsSmiLiteral()) {
-      return Bailout("SwitchStatement: non-literal switch label");
+
+    if (clause_type == none) {

[Vyacheslav Egorov (Chromium), 2011/10/24 10:56:26]

I would suggest extracting loop part that computes switch type into a separate
method just to make it cleaner.

+      if (clause->label()->IsSmiLiteral()) {
+        clause_type = smi;
+      } else if (clause->label()->IsStringLiteral()) {
+        clause_type = string;
+      } else {
+        return Bailout("SwitchStatement: non-literal switch label");
+      }
+    } else if (clause->label()->IsSmiLiteral() && clause_type == string ||

[Vyacheslav Egorov (Chromium), 2011/10/24 10:56:26]

clause can be neither smi nor string literal.

I think what you want here is 

(clause_type == kSmiSwitch && !clause->label()->IsSmiLiteral()) || ...

+               clause->label()->IsStringLiteral() && clause_type == smi) {
+      return Bailout("SwitchStatemnt: mixed label types are not supported");
     }
 
-    // Unconditionally deoptimize on the first non-smi compare.
     clause->RecordTypeFeedback(oracle());
-    if (!clause->IsSmiCompare()) {
-      // Finish with deoptimize and add uses of enviroment values to
-      // account for invisible uses.
-      current_block()->FinishExitWithDeoptimization(HDeoptimize::kUseAll);
-      set_current_block(NULL);
-      break;
-    }
 
-    // Otherwise generate a compare and branch.
+    // Generate a compare and branch.
     CHECK_ALIVE(VisitForValue(clause->label()));
     HValue* label_value = Pop();
-    HCompareIDAndBranch* compare =
-        new(zone()) HCompareIDAndBranch(tag_value,
-                                        label_value,
-                                        Token::EQ_STRICT);
-    compare->SetInputRepresentation(Representation::Integer32());
-    HBasicBlock* body_block = graph()->CreateBasicBlock();
+
     HBasicBlock* next_test_block = graph()->CreateBasicBlock();
+    HBasicBlock* body_block = graph()->CreateBasicBlock();
+
+    HControlInstruction* compare;
+
+    if (clause_type == smi) {
+      if (!clause->IsSmiCompare()) {
+        // Finish with deoptimize and add uses of enviroment values to
+        // account for invisible uses.
+        current_block()->FinishExitWithDeoptimization(HDeoptimize::kUseAll);
+        set_current_block(NULL);
+        break;
+      }
+
+      HCompareIDAndBranch* compare_ =
+          new(zone()) HCompareIDAndBranch(tag_value,
+                                          label_value,
+                                          Token::EQ_STRICT);
+      compare_->SetInputRepresentation(Representation::Integer32());
+      compare = reinterpret_cast<HControlInstruction*>(compare_);

[Vyacheslav Egorov (Chromium), 2011/10/24 10:56:26]

I don't think you need reinterpret_cast here.

HCompareObjectEqAndBranch* should be assignable to HControlInstruction.

+    } else {
+      HCompareObjectEqAndBranch* compare_ =

[Vyacheslav Egorov (Chromium), 2011/10/24 10:56:26]

I think you don't need this temp variable

+          new(zone()) HCompareObjectEqAndBranch(tag_value, label_value);

[Vyacheslav Egorov (Chromium), 2011/10/24 10:56:26]

Why not CompareIDAndBranch? Once loop that computes type is extracted you can
use computed type to emit is-symbol test before the switch.

+      compare = reinterpret_cast<HControlInstruction*>(compare_);

[Vyacheslav Egorov (Chromium), 2011/10/24 10:56:26]

I don't think you need reinterpret_cast here.

HCompareObjectEqAndBranch* should be assignable to HControlInstruction.

+    }
+
     compare->SetSuccessorAt(0, body_block);
     compare->SetSuccessorAt(1, next_test_block);
     current_block()->Finish(compare);
+
     set_current_block(next_test_block);
   }