[vm/compiler] Follow redefinitions in Value::NeedsWriteBarrier()

alexmarkov · commit-bot@chromium.org · commit 20407e28dbb0 · 2019-08-13T16:15:11.000Z
This is needed in order to account for redefinitions with less precise type, such as AssertAssignable of Smi to a generic T. x64, bytecode compiler: CryptoDecrypt(RunTime): 8516.8 -> 8190.6 us Change-Id: I8c5b8eb3efcc7ae1ccf204f5081772081be29c2f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/112755 Commit-Queue: Alexander Markov <alexmarkov@google.com> Reviewed-by: Martin Kustermann <kustermann@google.com>
diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc
@@ -1271,17 +1271,28 @@ void FlowGraphVisitor::VisitBlocks() {
 }
 
 bool Value::NeedsWriteBarrier() {
-  if (Type()->IsNull() || (Type()->ToNullableCid() == kSmiCid) ||
-      (Type()->ToNullableCid() == kBoolCid)) {
-    return false;
-  }
+  Value* value = this;
+  do {
+    if (value->Type()->IsNull() ||
+        (value->Type()->ToNullableCid() == kSmiCid) ||
+        (value->Type()->ToNullableCid() == kBoolCid)) {
+      return false;
+    }
 
-  // Strictly speaking, the incremental barrier can only be skipped for
-  // immediate objects (Smis) or permanent objects (vm-isolate heap or
-  // image pages). Here we choose to skip the barrier for any constant on
-  // the assumption it will remain reachable through the object pool.
+    // Strictly speaking, the incremental barrier can only be skipped for
+    // immediate objects (Smis) or permanent objects (vm-isolate heap or
+    // image pages). Here we choose to skip the barrier for any constant on
+    // the assumption it will remain reachable through the object pool.
+    if (value->BindsToConstant()) {
+      return false;
+    }
 
-  return !BindsToConstant();
+    // Follow the chain of redefinitions as redefined value could have a more
+    // accurate type (for example, AssertAssignable of Smi to a generic T).
+    value = value->definition()->RedefinedValue();
+  } while (value != nullptr);
+
+  return true;
 }
 
 void JoinEntryInstr::AddPredecessor(BlockEntryInstr* predecessor) {
diff --git a/runtime/vm/compiler/backend/il_test.cc b/runtime/vm/compiler/backend/il_test.cc
@@ -3,6 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/compiler/backend/il.h"
+#include "vm/compiler/backend/il_test_helper.h"
 #include "vm/unit_test.h"
 
 namespace dart {
@@ -40,4 +41,48 @@ ISOLATE_UNIT_TEST_CASE(OptimizationTests) {
   EXPECT(!c3->Equals(c1));
 }
 
+ISOLATE_UNIT_TEST_CASE(IRTest_EliminateWriteBarrier) {
+  const char* kScript =
+      R"(
+      class Container<T> {
+        operator []=(var index, var value) {
+          return data[index] = value;
+        }
+
+        List<T> data = new List<T>()..length = 10;
+      }
+
+      Container<int> x = new Container<int>();
+
+      foo() {
+        for (int i = 0; i < 10; ++i) {
+          x[i] = i;
+        }
+      }
+    )";
+
+  const auto& root_library = Library::Handle(LoadTestScript(kScript));
+  const auto& function = Function::Handle(GetFunction(root_library, "foo"));
+
+  Invoke(root_library, "foo");
+
+  TestPipeline pipeline(function, CompilerPass::kJIT);
+  FlowGraph* flow_graph = pipeline.RunPasses({});
+
+  auto entry = flow_graph->graph_entry()->normal_entry();
+  EXPECT(entry != nullptr);
+
+  StoreIndexedInstr* store_indexed = nullptr;
+
+  ILMatcher cursor(flow_graph, entry, true);
+  RELEASE_ASSERT(cursor.TryMatch({
+      kMoveGlob,
+      kMatchAndMoveBranchTrue,
+      kMoveGlob,
+      {kMatchStoreIndexed, &store_indexed},
+  }));
+
+  EXPECT(!store_indexed->value()->NeedsWriteBarrier());
+}
+
 }  // namespace dart
