SwitchOpt determined by AggressiveIntTypeSpec breaks repeated String cases

This change fixes an issue where the function GlobOpt::IsSwitchOptEnabled was updated to add conditions
for type specialization. This resulted in bad IR being produced, which impacted how type-specialized BailOuts
were constructed and which registers were restored (or, not restored). The fix is to apply align optimization
checks in SwitchIRBuilder based on the types of the cases.

CR Feedback fixes

Author: Thomas Moore (CHAKRA)

lib/Backend/GlobOpt.cpp

@@ -2917,7 +2917,7 @@ GlobOpt::TypeSpecializeBailoutExpectedInteger(IR::Instr* instr, Value* src1Val,
     {
         if (!src1Val || !src1Val->GetValueInfo()->IsLikelyInt() || instr->GetSrc1()->AsRegOpnd()->m_sym->m_isNotInt)
         {
-            Assert(IsSwitchOptEnabled());
+            Assert(IsSwitchOptEnabledForIntTypeSpec());
             throw Js::RejitException(RejitReason::DisableSwitchOptExpectingInteger);
         }
 
@@ -11365,7 +11365,7 @@ GlobOpt::ToTypeSpecUse(IR::Instr *instr, IR::Opnd *opnd, BasicBlock *block, Valu
                         // restarted with aggressive int type specialization disabled.
                         if(bailOutKind == IR::BailOutExpectingInteger)
                         {
-                            Assert(IsSwitchOptEnabled());
+                            Assert(IsSwitchOptEnabledForIntTypeSpec());
                             throw Js::RejitException(RejitReason::DisableSwitchOptExpectingInteger);
                         }
                         else
@@ -11709,7 +11709,7 @@ GlobOpt::ToTypeSpecUse(IR::Instr *instr, IR::Opnd *opnd, BasicBlock *block, Valu
                     // need to bail out.
                     if (bailOutKind == IR::BailOutExpectingInteger)
                     {
-                        Assert(IsSwitchOptEnabled());
+                        Assert(IsSwitchOptEnabledForIntTypeSpec());
                     }
                     else
                     {
@@ -17193,8 +17193,13 @@ bool
 GlobOpt::IsSwitchOptEnabled(Func const * func)
 {
     Assert(func->IsTopFunc());
-    return !PHASE_OFF(Js::SwitchOptPhase, func) && !func->IsSwitchOptDisabled() && !IsTypeSpecPhaseOff(func)
-        && DoAggressiveIntTypeSpec(func) && func->DoGlobOpt();
+    return !PHASE_OFF(Js::SwitchOptPhase, func) && !func->IsSwitchOptDisabled() && func->DoGlobOpt();
+}
+
+bool
+GlobOpt::IsSwitchOptEnabledForIntTypeSpec(Func const * func)
+{
+    return IsSwitchOptEnabled(func) && !IsTypeSpecPhaseOff(func) && DoAggressiveIntTypeSpec(func);
 }
 
 bool

lib/Backend/GlobOpt.h

@@ -764,6 +764,7 @@ class GlobOpt
     static bool             DoTypedArrayTypeSpec(Func const * func);
     static bool             DoNativeArrayTypeSpec(Func const * func);
     static bool             IsSwitchOptEnabled(Func const * func);
+    static bool             IsSwitchOptEnabledForIntTypeSpec(Func const * func);
     static bool             DoInlineArgsOpt(Func const * func);
     static bool             IsPREInstrCandidateLoad(Js::OpCode opcode);
     static bool             IsPREInstrCandidateStore(Js::OpCode opcode);
@@ -790,6 +791,7 @@ class GlobOpt
     bool                    DoNativeArrayTypeSpec() const { return GlobOpt::DoNativeArrayTypeSpec(this->func); }
     bool                    DoLdLenIntSpec(IR::Instr * const instr, const ValueType baseValueType);
     bool                    IsSwitchOptEnabled() const { return GlobOpt::IsSwitchOptEnabled(this->func); }
+    bool                    IsSwitchOptEnabledForIntTypeSpec() const { return GlobOpt::IsSwitchOptEnabledForIntTypeSpec(this->func); }
     bool                    DoPathDependentValues() const;
     bool                    DoTrackRelativeIntBounds() const;
     bool                    DoBoundCheckElimination() const;

lib/Backend/GlobOptBlockData.cpp

@@ -1790,7 +1790,7 @@ GlobOptBlockData::IsTypeSpecialized(Sym const * sym) const
 bool
 GlobOptBlockData::IsSwitchInt32TypeSpecialized(IR::Instr const * instr) const
 {
-    return GlobOpt::IsSwitchOptEnabled(instr->m_func->GetTopFunc())
+    return GlobOpt::IsSwitchOptEnabledForIntTypeSpec(instr->m_func->GetTopFunc())
         && instr->GetSrc1()->IsRegOpnd()
         && this->IsInt32TypeSpecialized(instr->GetSrc1()->AsRegOpnd()->m_sym);
 }

lib/Backend/SwitchIRBuilder.cpp

@@ -235,15 +235,16 @@ SwitchIRBuilder::OnCase(IR::RegOpnd * src1Opnd, IR::Opnd * src2Opnd, uint32 offs
     const bool isIntConst = src2Opnd->IsIntConstOpnd() || (sym && sym->IsIntConst());
     const bool isStrConst = !isIntConst && sym && sym->m_isStrConst;
 
-    if (GlobOpt::IsSwitchOptEnabled(m_func->GetTopFunc()) && 
-        isIntConst && 
-        m_intConstSwitchCases->TestAndSet(sym ? sym->GetIntConstValue() : src2Opnd->AsIntConstOpnd()->AsInt32()))
+    if (isIntConst
+        && GlobOpt::IsSwitchOptEnabledForIntTypeSpec(m_func->GetTopFunc())
+        && m_intConstSwitchCases->TestAndSet(sym ? sym->GetIntConstValue() : src2Opnd->AsIntConstOpnd()->AsInt32()))
     {
         // We've already seen a case statement with the same int const value. No need to emit anything for this.
         return;
     }
 
-    if (GlobOpt::IsSwitchOptEnabled(m_func->GetTopFunc()) && isStrConst
+    if (isStrConst
+        && GlobOpt::IsSwitchOptEnabled(m_func->GetTopFunc())
         && TestAndAddStringCaseConst(JITJavascriptString::FromVar(sym->GetConstAddress(true))))
     {
         // We've already seen a case statement with the same string const value. No need to emit anything for this.
@@ -253,36 +254,27 @@ SwitchIRBuilder::OnCase(IR::RegOpnd * src1Opnd, IR::Opnd * src2Opnd, uint32 offs
     branchInstr = IR::BranchInstr::New(m_eqOp, nullptr, src1Opnd, src2Opnd, m_func);
     branchInstr->m_isSwitchBr = true;
 
-    /*
     //  Switch optimization
     //  For Integers - Binary Search or jump table optimization technique is used
     //  For Strings - Dictionary look up technique is used.
     //
     //  For optimizing, the Load instruction corresponding to the switch instruction is profiled in the interpreter.
     //  Based on the dynamic profile data, optimization technique is decided.
-    */
-
-    bool deferred = false;
-
-    if (GlobOpt::IsSwitchOptEnabled(m_func->GetTopFunc()))
+    if (m_switchIntDynProfile && isIntConst && GlobOpt::IsSwitchOptEnabledForIntTypeSpec(m_func->GetTopFunc()))
     {
-        if (m_switchIntDynProfile && isIntConst)
-        {
-            CaseNode* caseNode = JitAnew(m_tempAlloc, CaseNode, branchInstr, offset, targetOffset, src2Opnd);
-            m_caseNodes->Add(caseNode);
-            deferred = true;
-        }
-        else if (m_switchStrDynProfile && isStrConst)
-        {
-            CaseNode* caseNode = JitAnew(m_tempAlloc, CaseNode, branchInstr, offset, targetOffset, src2Opnd);
-            m_caseNodes->Add(caseNode);
-            m_seenOnlySingleCharStrCaseNodes = m_seenOnlySingleCharStrCaseNodes && caseNode->GetUpperBoundStringConstLocal()->GetLength() == 1;
-            deferred = true;
-        }
+        CaseNode* caseNode = JitAnew(m_tempAlloc, CaseNode, branchInstr, offset, targetOffset, src2Opnd);
+        m_caseNodes->Add(caseNode);
     }
-
-    if (!deferred)
+    else if (m_switchStrDynProfile && isStrConst && GlobOpt::IsSwitchOptEnabled(m_func->GetTopFunc()))
+    {
+        CaseNode* caseNode = JitAnew(m_tempAlloc, CaseNode, branchInstr, offset, targetOffset, src2Opnd);
+        m_caseNodes->Add(caseNode);
+        m_seenOnlySingleCharStrCaseNodes = m_seenOnlySingleCharStrCaseNodes && caseNode->GetUpperBoundStringConstLocal()->GetLength() == 1;
+    }
+    else
     {
+        // Otherwise, there are no optimizations to defer, so add the branch for
+        // this case instruction now
         FlushCases(offset);
         m_adapter->AddBranchInstr(branchInstr, offset, targetOffset);
     }

lib/Backend/SwitchIRBuilder.h

@@ -60,7 +60,9 @@ struct IRBuilderAsmJsSwitchAdapter : public SwitchAdapter {
 #endif
 
 /**
- * Handles construction of switch statements, with appropriate optimizations
+ * Handles construction of switch statements, with appropriate optimizations. Note that some of these
+ * optimizations occur during IR building (rather than GlobOpt) because the abstraction of a switch/case
+ * block is not maintained with the resulting IR. Thus, some optimizations must occur during this phase.
  */
 class SwitchIRBuilder {
 private: