Fix StringBuilder merging

Description: StringBuilder merge optimization mistakenly merges two builder when the way they used is not supported

Issue: #IB1IO8
Tests: All required pre-merge test passed Results are available in the ggwatcher
Signed-off-by: Mikhail Ivanov <ivanov.mikhail2@huawei.com>

static_core/compiler/optimizer/optimizations/simplify_string_builder.cpp

@@ -25,7 +25,6 @@
 #include "optimizer/ir/inst.h"
 
 #include "optimizer/optimizations/cleanup.h"
-#include "optimizer/optimizations/string_builder_utils.h"
 
 namespace ark::compiler {
 
@@ -125,26 +124,6 @@ bool IsDataFlowInput(Inst *inst, Inst *input)
     return false;
 }
 
-bool IsUsedOutsideBasicBlock(Inst *inst, BasicBlock *bb)
-{
-    for (auto &user : inst->GetUsers()) {
-        auto userInst = user.GetInst();
-        if (userInst->IsCheck()) {
-            if (!userInst->HasSingleUser()) {
-                // In case of multi user check-instruction we assume it is used outside current basic block without
-                // actually testing it.
-                return true;
-            }
-            // In case of single user check-instruction we test its the only user.
-            userInst = userInst->GetUsers().Front().GetInst();
-        }
-        if (userInst->GetBasicBlock() != bb) {
-            return true;
-        }
-    }
-    return false;
-}
-
 void SimplifyStringBuilder::OptimizeStringBuilderToString(BasicBlock *block)
 {
     // Removes unnecessary String Builder instances
@@ -302,8 +281,15 @@ bool SimplifyStringBuilder::MatchConcatenation(InstIter &begin, const InstIter &
         }
     }
 
-    // Supported case: number of toString-calls is one,
+    for (size_t index = 0; index < match.appendCount; ++index) {
-    // number of append calls is between 2 and 4
+        if (!match.appendIntrinsics[index]->IsDominate(match.toStringCall)) {
+            return false;
+        }
+    }
+
+    // Supported case: number of toString calls is one,
+    // number of append calls is between 2 and 4,
+    // toString call is dominated by all append calls.
     return true;
 }
 
@@ -537,21 +523,6 @@ SaveStateInst *FindPreHeaderSaveState(Loop *loop)
     return nullptr;
 }
 
-SaveStateInst *FindFirstSaveState(BasicBlock *block)
-{
-    if (block->IsEmpty()) {
-        return nullptr;
-    }
-
-    for (auto inst : block->Insts()) {
-        if (inst->GetOpcode() == Opcode::SaveState) {
-            return inst->CastToSaveState();
-        }
-    }
-
-    return nullptr;
-}
-
 size_t CountOuterLoopSuccs(BasicBlock *block)
 {
     return std::count_if(block->GetSuccsBlocks().begin(), block->GetSuccsBlocks().end(),
@@ -654,26 +625,6 @@ ArenaVector<Inst *> SimplifyStringBuilder::FindStringBuilderAppendInstructions(I
     return appendInstructions;
 }
 
-void RemoveFromInstructionInputs(ArenaVector<std::pair<Inst *, size_t>> &inputDescriptors)
-{
-    // Inputs must be walked in reverse order for removal
-    std::sort(inputDescriptors.begin(), inputDescriptors.end(),
-              [](auto inputDescX, auto inputDescY) { return inputDescX.second > inputDescY.second; });
-
-    for (auto inputDesc : inputDescriptors) {
-        auto inst = inputDesc.first;
-        auto index = inputDesc.second;
-
-        [[maybe_unused]] auto inputInst = inst->GetInput(index).GetInst();
-        COMPILER_LOG(DEBUG, SIMPLIFY_SB) << "Remove input id=" << inputInst->GetId() << " ("
-                                         << GetOpcodeString(inputInst->GetOpcode())
-                                         << ") from instruction id=" << inst->GetId() << " ("
-                                         << GetOpcodeString(inst->GetOpcode()) << ")";
-
-        inst->RemoveInput(index);
-    }
-}
-
 void SimplifyStringBuilder::RemoveFromSaveStateInputs(Inst *inst)
 {
     inputDescriptors_.clear();
@@ -1789,29 +1740,6 @@ void SimplifyStringBuilder::ReplaceWithAppendIntrinsic(const ConcatenationMatch
     isApplied_ = true;
 }
 
-bool BreakStringBuilderAppendChains(BasicBlock *block)
-{
-    // StringBuilder append-call returns 'this' (instance)
-    // Replace all users of append-call with instance itself to support chain calls
-    // like: sb.append(s0).append(s1)...
-    bool isApplied = false;
-    for (auto inst : block->Insts()) {
-        if (!IsStringBuilderAppend(inst) && !IsStringBuilderToString(inst)) {
-            continue;
-        }
-
-        auto instance = inst->GetDataFlowInput(0);
-        for (auto &user : instance->GetUsers()) {
-            auto userInst = SkipSingleUserCheckInstruction(user.GetInst());
-            if (IsStringBuilderAppend(userInst)) {
-                userInst->ReplaceUsers(instance);
-                isApplied = true;
-            }
-        }
-    }
-    return isApplied;
-}
-
 bool HasPhiOfStringBuilders(BasicBlock *block)
 {
     for (auto phi : block->PhiInsts()) {
@@ -1946,30 +1874,27 @@ void SimplifyStringBuilder::CollectStringBuilderLastCalls(BasicBlock *block)
             continue;
         }
 
-        if (IsMethodStringBuilderDefaultConstructor(inst) || IsMethodStringBuilderConstructorWithCharArrayArg(inst) ||
+        if (!IsStringBuilderToString(inst)) {
-            IsMethodStringBuilderConstructorWithStringArg(inst)) {
             continue;
         }
 
-        for (size_t index = 0; index < inst->GetInputsCount(); ++index) {
+        auto inputInst = inst->GetDataFlowInput(0);
-            auto inputInst = inst->GetDataFlowInput(index);
+        if (inputInst->IsSaveState()) {
-            if (inputInst->IsSaveState()) {
+            continue;
-                continue;
+        }
-            }
 
-            auto instance = inputInst;
+        auto instance = inputInst;
-            if (!IsStringBuilderInstance(instance)) {
+        if (!IsStringBuilderInstance(instance)) {
-                continue;
+            continue;
-            }
+        }
 
-            auto calls = stringBuilderFirstLastCalls_.find(instance);
+        auto calls = stringBuilderFirstLastCalls_.find(instance);
-            if (calls == stringBuilderFirstLastCalls_.end()) {
+        if (calls == stringBuilderFirstLastCalls_.end()) {
-                calls = stringBuilderFirstLastCalls_.emplace(instance, InstPair {}).first;
+            calls = stringBuilderFirstLastCalls_.emplace(instance, InstPair {}).first;
-            }
+        }
-            auto &lastCall = calls->second.second;
+        auto &lastCall = calls->second.second;
-            if (lastCall == nullptr) {
+        if (lastCall == nullptr) {
-                lastCall = inst;
+            lastCall = inst;
-            }
         }
     }
 }
@@ -1998,8 +1923,8 @@ bool SimplifyStringBuilder::CanMergeStringBuilders(Inst *instance, const InstPai
         return false;
     }
 
-    auto &[inputInstnceFirstAppendCall, inputInstanceLastToStringCall] = inputInstanceCalls->second;
+    auto &[inputInstanceFirstAppendCall, inputInstanceLastToStringCall] = inputInstanceCalls->second;
-    if (inputInstnceFirstAppendCall == nullptr || inputInstanceLastToStringCall == nullptr) {
+    if (inputInstanceFirstAppendCall == nullptr || inputInstanceLastToStringCall == nullptr) {
         return false;  // Unsupported case: doesn't look like concatenation pattern
     }
 
@@ -2019,6 +1944,18 @@ bool SimplifyStringBuilder::CanMergeStringBuilders(Inst *instance, const InstPai
         return false;
     }
 
+    // Check if 'inputInstance' toString call comes after any 'inputInstance' append call
+    for (auto &user : inputInstance->GetUsers()) {
+        auto userInst = SkipSingleUserCheckInstruction(user.GetInst());
+        if (!IsStringBuilderAppend(userInst)) {
+            continue;
+        }
+
+        if (inputInstanceLastToStringCall->IsDominate(userInst)) {
+            return false;
+        }
+    }
+
     // Check if all 'inputInstance' calls comes before any 'instance' call
     return inputInstanceLastToStringCall->IsDominate(instanceFirstCall);
 }
@@ -2220,7 +2157,10 @@ void SimplifyStringBuilder::OptimizeStringBuilderChain()
         auto &calls = instanceCalls.second;
 
         for (auto call : calls) {
-            call->SetInput(0U, inputInstance);
+            // Switch call to new instance only if it is not marked for removal
+            if (call->GetFlag(inst_flags::NO_DCE)) {
+                call->SetInput(0U, inputInstance);
+            }
         }
 
         FixBrokenSaveStatesForStringBuilderCalls(inputInstance);

static_core/compiler/optimizer/optimizations/simplify_string_builder.h

@@ -22,6 +22,7 @@
 #include "optimizer/ir/graph.h"
 #include "optimizer/ir/inst.h"
 #include "optimizer/pass.h"
+#include "optimizer/optimizations/string_builder_utils.h"
 
 namespace ark::compiler {
 
@@ -273,7 +274,7 @@ private:
     SaveStateBridgesBuilder ssb_ {};
     ArenaStack<Inst *> instructionsStack_;
     ArenaVector<Inst *> instructionsVector_;
-    ArenaVector<std::pair<Inst *, size_t>> inputDescriptors_;
+    ArenaVector<InputDesc> inputDescriptors_;
     ArenaVector<StringBuilderUsage> usages_;
     ArenaVector<ConcatenationLoopMatch> matches_;
     StringBuilderCallsMap stringBuilderCalls_;

static_core/compiler/optimizer/optimizations/string_builder_utils.cpp

@@ -273,4 +273,78 @@ bool IsIntrinsicStringBuilderAppendString(Inst *inst)
     return runtime->IsIntrinsicStringBuilderAppendString(inst->CastToIntrinsic()->GetIntrinsicId());
 }
 
+bool IsUsedOutsideBasicBlock(Inst *inst, BasicBlock *bb)
+{
+    for (auto &user : inst->GetUsers()) {
+        auto userInst = user.GetInst();
+        if (userInst->IsCheck()) {
+            if (!userInst->HasUsers()) {
+                continue;
+            }
+            if (!userInst->HasSingleUser()) {
+                // In case of multi user check-instruction we assume it is used outside current basic block without
+                // actually testing it.
+                return true;
+            }
+            // In case of single user check-instruction we test its the only user.
+            userInst = userInst->GetUsers().Front().GetInst();
+        }
+        if (userInst->GetBasicBlock() != bb) {
+            return true;
+        }
+    }
+    return false;
+}
+
+SaveStateInst *FindFirstSaveState(BasicBlock *block)
+{
+    if (block->IsEmpty()) {
+        return nullptr;
+    }
+
+    for (auto inst : block->Insts()) {
+        if (inst->GetOpcode() == Opcode::SaveState) {
+            return inst->CastToSaveState();
+        }
+    }
+
+    return nullptr;
+}
+
+void RemoveFromInstructionInputs(ArenaVector<InputDesc> &inputDescriptors)
+{
+    // Inputs must be walked in reverse order for removal
+    std::sort(inputDescriptors.begin(), inputDescriptors.end(),
+              [](auto inputDescX, auto inputDescY) { return inputDescX.second > inputDescY.second; });
+
+    for (auto inputDesc : inputDescriptors) {
+        auto inst = inputDesc.first;
+        auto index = inputDesc.second;
+        inst->RemoveInput(index);
+    }
+}
+
+bool BreakStringBuilderAppendChains(BasicBlock *block)
+{
+    // StringBuilder append-call returns 'this' (instance)
+    // Replace all users of append-call with instance itself to support chain calls
+    // like: sb.append(s0).append(s1)...
+    bool isApplied = false;
+    for (auto inst : block->Insts()) {
+        if (!IsStringBuilderAppend(inst) && !IsStringBuilderToString(inst)) {
+            continue;
+        }
+
+        auto instance = inst->GetDataFlowInput(0);
+        for (auto &user : instance->GetUsers()) {
+            auto userInst = SkipSingleUserCheckInstruction(user.GetInst());
+            if (IsStringBuilderAppend(userInst)) {
+                userInst->ReplaceUsers(instance);
+                isApplied = true;
+            }
+        }
+    }
+    return isApplied;
+}
+
 }  // namespace ark::compiler

static_core/compiler/optimizer/optimizations/string_builder_utils.h

@@ -44,6 +44,8 @@ bool HasUserPhiRecursively(Inst *inst, Marker visited, const FindUserPredicate &
 size_t CountUsers(Inst *inst, const FindUserPredicate &predicate);
 void ResetUserMarkersRecursively(Inst *inst, Marker visited);
 Inst *SkipSingleUserCheckInstruction(Inst *inst);
+bool IsUsedOutsideBasicBlock(Inst *inst, BasicBlock *bb);
+SaveStateInst *FindFirstSaveState(BasicBlock *block);
 
 template <bool ALLOW_INLINED = false>
 bool IsStringBuilderAppend(Inst *inst)
@@ -62,6 +64,11 @@ bool IsStringBuilderAppend(Inst *inst)
 }
 
 bool IsIntrinsicStringBuilderAppendString(Inst *inst);
+
+using InputDesc = std::pair<Inst *, unsigned>;
+void RemoveFromInstructionInputs(ArenaVector<InputDesc> &inputDescriptors);
+bool BreakStringBuilderAppendChains(BasicBlock *block);
+
 }  // namespace ark::compiler
 
 #endif  // COMPILER_OPTIMIZER_OPTIMIZATIONS_STRING_BUILDER_UTILS_H

static_core/plugins/ets/tests/checked/ets_string_builder_merge.sts

@@ -181,6 +181,26 @@
 //! INST_COUNT    "Intrinsic.StdCoreSbAppendString ",1
 //! PASS_AFTER    "Codegen"
 //! INST_COUNT    "Intrinsic.StdCoreSbToString",1
+//!
+//! METHOD        "ETSGLOBAL::unsupported_usage_chain_concat17"
+//! PASS_BEFORE   "BranchElimination"
+//! INST_COUNT    "StringBuilder::<ctor>",1
+//! INST_COUNT    "Intrinsic.StdCoreSbAppendString ",2
+//! INST_COUNT    "Intrinsic.StdCoreSbToString",1
+//! PASS_AFTER    "SimplifyStringBuilder"
+//! INST_COUNT    "StringBuilder::<ctor>",1
+//! INST_COUNT    "Intrinsic.StdCoreSbAppendString ",2
+//! INST_COUNT    "Intrinsic.StdCoreSbToString",1
+//!
+//! METHOD        "ETSGLOBAL::unsupported_usage_chain_concat18"
+//! PASS_BEFORE   "BranchElimination"
+//! INST_COUNT    "StringBuilder::<ctor>",1
+//! INST_COUNT    "Intrinsic.StdCoreSbAppendString ",2
+//! INST_COUNT    "Intrinsic.StdCoreSbToString",1
+//! PASS_AFTER    "SimplifyStringBuilder"
+//! INST_COUNT    "StringBuilder::<ctor>",1
+//! INST_COUNT    "Intrinsic.StdCoreSbAppendString ",2
+//! INST_COUNT    "Intrinsic.StdCoreSbToString",1
 
 //! CHECKER       JIT IR Builder, check String Builders merging
 //! SKIP_IF       @architecture == "arm32"
@@ -349,6 +369,28 @@
 //! INST_COUNT    "Intrinsic.StdCoreSbAppendString ",1
 //! PASS_AFTER    "Codegen"
 //! INST_COUNT    "Intrinsic.StdCoreSbToString",1
+//!
+//! METHOD        "ETSGLOBAL::unsupported_usage_chain_concat17"
+//! PASS_BEFORE   "BranchElimination"
+//! INST_COUNT    "StringBuilder::<ctor>",1
+//! INST_COUNT    "Intrinsic.StdCoreSbAppendString ",2
+//! INST_COUNT    "Intrinsic.StdCoreSbToString",1
+//! PASS_AFTER    "SimplifyStringBuilder"
+//! INST_COUNT    "StringBuilder::<ctor>",1
+//! INST_COUNT    "Intrinsic.StdCoreSbAppendString ",2
+//! INST_COUNT    "Intrinsic.StdCoreSbToString",1
+//!
+//! METHOD        "ETSGLOBAL::unsupported_usage_chain_concat18"
+//! PASS_BEFORE   "BranchElimination"
+//! INST_COUNT    "StringBuilder::<ctor>",1
+//! INST_COUNT    "Intrinsic.StdCoreSbAppendString ",2
+//! INST_COUNT    "Intrinsic.StdCoreSbToString",1
+//! INST_COUNT    "String::concat",1
+//! PASS_AFTER    "SimplifyStringBuilder"
+//! INST_COUNT    "StringBuilder::<ctor>",2
+//! INST_COUNT    "Intrinsic.StdCoreSbAppendString ",3
+//! INST_COUNT    "Intrinsic.StdCoreSbToString",2
+//! INST_NOT      "String::concat"
 
 // Each '+=' operation on strings in tests below is translated into StringBuilder + 2 append calls by frontend
 
@@ -628,6 +670,21 @@ function partially_supported_chain_concat16(a: String, b: String, c: String, d:
     return abcd;                                    // applied, 3 SBs merged into 1 SB + concat2
 }
 
+function unsupported_usage_chain_concat17(a: String): String {
+    let sb = new StringBuilder();
+    sb.append(a);
+    let result = sb.toString();
+    sb.append(a);
+    return result;                                  // not applied, sb must not be used after sb.toString() appended
+}
+
+function unsupported_usage_chain_concat18(a: String): String {
+    let sb = new StringBuilder();
+    sb.append(a);
+    let result = sb.toString();
+    sb.append(a);
+    return result.concat();                         // not applied, sb must not be used after sb.toString() appended
+}
 
 //! CHECKER       AOT IR Builder, check String Builders merging (chain_concat19 only, with --compiler-inlining=true)
 //! SKIP_IF       @architecture == "arm32"
@@ -686,6 +743,8 @@ function main() {
     assert partially_supported_chain_concat14("ab", "c", "d", "e") == "abcde": "Wrong result at partially_supported_chain_concat14";
     assert supported_chain_concat15("ab", "c", "d", "e") == "abcde": "Wrong result at supported_chain_concat15";
     assert partially_supported_chain_concat16("ab", "c", "d", "e") == "abcde": "Wrong result at partially_supported_chain_concat16";
+    assert unsupported_usage_chain_concat17("abc") == "abc": "Wrong result at unsupported_usage_chain_concat17";
+    assert unsupported_usage_chain_concat18("abc") == "abc": "Wrong result at unsupported_usage_chain_concat18";
 
     chain_concat19();
 }