Skip to content

Commit

Permalink
Reland "[vm/compiler] Always allow CSE of LoadStaticField & loosen as…
Browse files Browse the repository at this point in the history
…sertion 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 dart-lang/sdk#45133
Closes dart-lang/sdk#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>
  • Loading branch information
mkustermann authored and commit-bot@chromium.org committed Mar 25, 2021
1 parent 9847e7f commit 1453eeb
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 2 deletions.
3 changes: 2 additions & 1 deletion runtime/vm/compiler/backend/il.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1132,7 +1132,8 @@ bool LoadFieldInstr::AttributesEqual(Instruction* other) const {
}

bool LoadStaticFieldInstr::AttributesEqual(Instruction* other) const {
ASSERT(IsFieldInitialized());
ASSERT(AllowsCSE());
ASSERT(!field().is_late() || calls_initializer());
return field().ptr() == other->AsLoadStaticField()->field().ptr();
}

Expand Down
9 changes: 8 additions & 1 deletion runtime/vm/compiler/backend/il.h
Original file line number Diff line number Diff line change
Expand Up @@ -5523,7 +5523,14 @@ class LoadStaticFieldInstr : public TemplateDefinition<0, Throws> {
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 {
Expand Down

0 comments on commit 1453eeb

Please sign in to comment.