commit 1453eebebcc8b881ddb18d25573ad245b4c47c94
author Martin Kustermann <kustermann@google.com> Thu Mar 25 15:04:43 2021 +0000
committer commit-bot@chromium.org <commit-bot@chromium.org> Thu Mar 25 15:04:43 2021 +0000
tree 8fd553b95a7e1b517268ba64156586bd9609a3d3
parent 9847e7fbe62a1e4104228922885926c7a92cacb1

Reland "[vm/compiler] Always allow CSE of LoadStaticField & loosen assertion in AllowsCSE"

Loosening of assertion:

There is no guarantee that a static-final field has been initialized by
the time a function is compiled (in optimized mode) that uses such a
field.

We should therefore loosen the ASSERT to not require the field to be
initialized and rather allows CSE

Enabling of CSE:

In the past we had separate InitStaticField and LoadStaticField. The
load itself had no side-effects and could therefore be moved
arbitrarily. Though we couldn't allow it to be moved before it's
InitStaticField. This dependency was not explicitly made and we
therefore disabled CSE / LICM if the actual field was not initialized
(or field may be reset) - see [0].

Though after merging of InitStaticField and LoadStaticField in [1] there
is no longer a need for tracking any dependencies: The side-effects
of InitStaticField are now reported by LoadStaticField.
=> We can therefore always allow CSE of LoadStaticFieldinstr and
any code motion would respect side-effects of the instruction.

[0] https://codereview.chromium.org/1497783002
[1] https://dart-review.googlesource.com/c/sdk/+/148283

TEST=Fixes flaky test.

Closes https://github.com/dart-lang/sdk/issues/45133
Closes https://github.com/dart-lang/sdk/issues/45446

Change-Id: I4a699e9b1dc9dfec9a91208ed78ed0a0c41f5cad
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192924
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>

runtime/vm/compiler/backend/il.cc

diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc
index 6e617cd..5c5db00 100644
--- a/runtime/vm/compiler/backend/il.cc
+++ b/runtime/vm/compiler/backend/il.cc
@@ -1132,7 +1132,8 @@
 }
 
 bool LoadStaticFieldInstr::AttributesEqual(Instruction* other) const {
-  ASSERT(IsFieldInitialized());
+  ASSERT(AllowsCSE());
+  ASSERT(!field().is_late() || calls_initializer());
   return field().ptr() == other->AsLoadStaticField()->field().ptr();
 }
 

runtime/vm/compiler/backend/il.h

diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h
index 73159ee..7befee0 100644
--- a/runtime/vm/compiler/backend/il.h
+++ b/runtime/vm/compiler/backend/il.h
@@ -5523,7 +5523,14 @@
   void set_calls_initializer(bool value) { calls_initializer_ = value; }
 
   virtual bool AllowsCSE() const {
-    return field().is_final() && !FLAG_fields_may_be_reset;
+    // If two loads of a static-final-late field call the initializer and one
+    // dominates another, we can remove the dominated load with the result of
+    // the dominating load.
+    //
+    // Though if the field is final-late there can be stores into it via
+    // load/compare-with-sentinel/store. Those loads have `!calls_initializer()`
+    // and we won't allow CSE for them.
+    return field().is_final() && (!field().is_late() || calls_initializer());
   }
 
   virtual bool ComputeCanDeoptimize() const {