[hole-check-elimination] Simplest possible hole check elimination

doc: https://docs.google.com/document/d/1Y9uF3hS2aUrwKU56vGxlvEs_IiGgmWSzau8097Y-XBM/edit

Bug: v8:7427
Change-Id: Iedd36c146cefff7e6687fdad48d263889c5c8347
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1778902
Commit-Queue: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63913}

src/ast/scopes.cc

@@ -195,6 +195,9 @@ DeclarationScope::DeclarationScope(Zone* zone, ScopeType scope_type,
     DCHECK(!is_eval_scope());
     sloppy_eval_can_extend_vars_ = true;
   }
+  if (scope_info->CanElideThisHoleChecks()) {
+    can_elide_this_hole_checks_ = true;
+  }
 }
 
 Scope::Scope(Zone* zone, const AstRawString* catch_variable_name,
@@ -228,6 +231,7 @@ void DeclarationScope::SetDefaults() {
   scope_uses_super_property_ = false;
   has_checked_syntax_ = false;
   has_this_reference_ = false;
+  can_elide_this_hole_checks_ = false;
   has_this_declaration_ =
       (is_function_scope() && !is_arrow_scope()) || is_module_scope();
   needs_private_name_context_chain_recalc_ = false;

src/ast/scopes.h

@@ -1083,11 +1083,13 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope {
 
   void set_has_this_reference() { has_this_reference_ = true; }
   bool has_this_reference() const { return has_this_reference_; }
-  void UsesThis() {
-    set_has_this_reference();
-    GetReceiverScope()->receiver()->ForceContextAllocation();
+
+  bool can_elide_this_hole_checks() const {
+    return can_elide_this_hole_checks_;
   }
 
+  void set_can_elide_this_hole_checks() { can_elide_this_hole_checks_ = true; }
+
   bool needs_private_name_context_chain_recalc() const {
     return needs_private_name_context_chain_recalc_;
   }
@@ -1137,6 +1139,7 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope {
   bool has_checked_syntax_ : 1;
   bool has_this_reference_ : 1;
   bool has_this_declaration_ : 1;
+  bool can_elide_this_hole_checks_ : 1;
   bool needs_private_name_context_chain_recalc_ : 1;
 
   // If the scope is a function scope, this is the function kind.

src/diagnostics/objects-printer.cc

@@ -2303,6 +2303,9 @@ void ScopeInfo::ScopeInfoPrint(std::ostream& os) {  // NOLINT
   if (HasInferredFunctionName()) {
     os << "\n - inferred function name: " << Brief(InferredFunctionName());
   }
+  if (CanElideThisHoleChecks()) {
+    os << "\n - can elide this hole checks";
+  }
 
   if (HasPositionInfo()) {
     os << "\n - start position: " << StartPosition();

src/interpreter/bytecode-generator.cc

@@ -5583,9 +5583,9 @@ void BytecodeGenerator::VisitTemplateLiteral(TemplateLiteral* expr) {
 void BytecodeGenerator::BuildThisVariableLoad() {
   DeclarationScope* receiver_scope = closure_scope()->GetReceiverScope();
   Variable* var = receiver_scope->receiver();
-  // TODO(littledan): implement 'this' hole check elimination.
   HoleCheckMode hole_check_mode =
-      IsDerivedConstructor(receiver_scope->function_kind())
+      (IsDerivedConstructor(receiver_scope->function_kind()) &&
+       !receiver_scope->can_elide_this_hole_checks())
           ? HoleCheckMode::kRequired
           : HoleCheckMode::kElided;
   BuildVariableLoad(var, hole_check_mode);

src/objects/scope-info.cc

@@ -168,6 +168,7 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope,
     bool has_simple_parameters = false;
     bool is_asm_module = false;
     bool sloppy_eval_can_extend_vars = false;
+    bool can_elide_this_hole_checks = false;
     if (scope->is_function_scope()) {
       DeclarationScope* function_scope = scope->AsDeclarationScope();
       has_simple_parameters = function_scope->has_simple_parameters();
@@ -178,6 +179,8 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope,
       function_kind = scope->AsDeclarationScope()->function_kind();
       sloppy_eval_can_extend_vars =
           scope->AsDeclarationScope()->sloppy_eval_can_extend_vars();
+      can_elide_this_hole_checks =
+          scope->AsDeclarationScope()->can_elide_this_hole_checks();
     }
 
     // Encode the flags.
@@ -199,7 +202,8 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope,
         ForceContextAllocationField::encode(
             scope->ForceContextForLanguageMode()) |
         PrivateNameLookupSkipsOuterClassField::encode(
-            scope->private_name_lookup_skips_outer_class());
+            scope->private_name_lookup_skips_outer_class()) |
+        CanElideThisHoleChecksField::encode(can_elide_this_hole_checks);
     scope_info.SetFlags(flags);
 
     scope_info.SetParameterCount(parameter_count);
@@ -374,7 +378,8 @@ Handle<ScopeInfo> ScopeInfo::CreateForWithScope(
       HasOuterScopeInfoField::encode(has_outer_scope_info) |
       IsDebugEvaluateScopeField::encode(false) |
       ForceContextAllocationField::encode(false) |
-      PrivateNameLookupSkipsOuterClassField::encode(false);
+      PrivateNameLookupSkipsOuterClassField::encode(false) |
+      CanElideThisHoleChecksField::encode(false);
   scope_info->SetFlags(flags);
 
   scope_info->SetParameterCount(0);
@@ -441,7 +446,8 @@ Handle<ScopeInfo> ScopeInfo::CreateForBootstrapping(Isolate* isolate,
       HasOuterScopeInfoField::encode(false) |
       IsDebugEvaluateScopeField::encode(false) |
       ForceContextAllocationField::encode(false) |
-      PrivateNameLookupSkipsOuterClassField::encode(false);
+      PrivateNameLookupSkipsOuterClassField::encode(false) |
+      CanElideThisHoleChecksField::encode(false);
   scope_info->SetFlags(flags);
   scope_info->SetParameterCount(parameter_count);
   scope_info->SetContextLocalCount(context_local_count);
@@ -624,6 +630,11 @@ bool ScopeInfo::PrivateNameLookupSkipsOuterClass() const {
   return PrivateNameLookupSkipsOuterClassField::decode(Flags());
 }
 
+bool ScopeInfo::CanElideThisHoleChecks() const {
+  if (length() == 0) return false;
+  return CanElideThisHoleChecksField::decode(Flags());
+}
+
 bool ScopeInfo::HasContext() const { return ContextLength() > 0; }
 
 Object ScopeInfo::FunctionName() const {

src/objects/scope-info.h

@@ -53,6 +53,9 @@ class ScopeInfo : public FixedArray {
   // Does this scope make a sloppy eval call?
   bool SloppyEvalCanExtendVars() const;
 
+  // True if we can elide 'this' hole checks in this scope.
+  bool CanElideThisHoleChecks() const;
+
   // Return the number of context slots for code if a context is allocated. This
   // number consists of three parts:
   //  1. Size of fixed header for every context: Context::MIN_CONTEXT_SLOTS
@@ -250,6 +253,8 @@ class ScopeInfo : public FixedArray {
   using ForceContextAllocationField = IsDebugEvaluateScopeField::Next<bool, 1>;
   using PrivateNameLookupSkipsOuterClassField =
       ForceContextAllocationField::Next<bool, 1>;
+  using CanElideThisHoleChecksField =
+      PrivateNameLookupSkipsOuterClassField::Next<bool, 1>;
 
   STATIC_ASSERT(kLastFunctionKind <= FunctionKindField::kMax);
 

src/parsing/expression-scope.h

@@ -127,6 +127,16 @@ class ExpressionScope {
     } while (scope != nullptr);
   }
 
+  void RecordCallsSuper() {
+    ExpressionScope* scope = this;
+    do {
+      if (scope->IsArrowHeadParsingScope()) {
+        scope->AsArrowHeadParsingScope()->RecordCallsSuper();
+      }
+      scope = scope->parent();
+    } while (scope != nullptr);
+  }
+
   void RecordPatternError(const Scanner::Location& loc,
                           MessageTemplate message) {
     // TODO(verwaest): Non-assigning expression?
@@ -760,7 +770,12 @@ class ArrowHeadParsingScope : public ExpressionParsingScope<Types> {
     }
 #endif  // DEBUG
 
-    if (uses_this_) result->UsesThis();
+    if (uses_this_) {
+      result->set_has_this_reference();
+    }
+    if (uses_this_ || calls_super_) {
+      result->GetReceiverScope()->receiver()->ForceContextAllocation();
+    }
     return result;
   }
 
@@ -773,6 +788,7 @@ class ArrowHeadParsingScope : public ExpressionParsingScope<Types> {
 
   void RecordNonSimpleParameter() { has_simple_parameter_list_ = false; }
   void RecordThisUse() { uses_this_ = true; }
+  void RecordCallsSuper() { calls_super_ = true; }
 
  private:
   FunctionKind kind() const {
@@ -785,6 +801,7 @@ class ArrowHeadParsingScope : public ExpressionParsingScope<Types> {
   MessageTemplate declaration_error_message = MessageTemplate::kNone;
   bool has_simple_parameter_list_ = true;
   bool uses_this_ = false;
+  bool calls_super_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(ArrowHeadParsingScope);
 };

src/parsing/parser-base.h

@@ -998,6 +998,21 @@ class ParserBase {
     return var;
   }
 
+  // Similar to UseThis, but does not disable hole check elision.
+  V8_INLINE void CallsSuper() {
+    DeclarationScope* closure_scope = scope()->GetClosureScope();
+    DeclarationScope* receiver_scope = closure_scope->GetReceiverScope();
+    if (closure_scope == receiver_scope) {
+      // It's possible that we're parsing the head of an arrow function, in
+      // which case we haven't realized yet that closure_scope !=
+      // receiver_scope. Mark through the ExpressionScope for now.
+      expression_scope()->RecordCallsSuper();
+    } else {
+      Variable* var = receiver_scope->receiver();
+      var->ForceContextAllocation();
+    }
+  }
+
   V8_INLINE IdentifierT ParseAndClassifyIdentifier(Token::Value token);
   // Parses an identifier or a strict mode future reserved word. Allows passing
   // in function_kind for the case of parsing the identifier in a function
@@ -1091,6 +1106,8 @@ class ParserBase {
 
   ExpressionT ParseArrowFunctionLiteral(const FormalParametersT& parameters);
   void ParseAsyncFunctionBody(Scope* scope, StatementListT* body);
+  void ParseDerivedConstructorBody(StatementListT* body,
+                                   Token::Value end_token);
   ExpressionT ParseAsyncFunctionLiteral();
   ExpressionT ParseClassLiteral(IdentifierT name,
                                 Scanner::Location class_name_location,
@@ -3473,8 +3490,7 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseSuperExpression(
     if (!is_new && peek() == Token::LPAREN && IsDerivedConstructor(kind)) {
       // TODO(rossberg): This might not be the correct FunctionState for the
       // method here.
-      expression_scope()->RecordThisUse();
-      UseThis();
+      CallsSuper();
       return impl()->NewSuperCallReference(pos);
     }
   }
@@ -4045,16 +4061,18 @@ void ParserBase<Impl>::ParseFunctionBody(
         impl()->ParseAndRewriteGeneratorFunctionBody(pos, kind, &inner_body);
       } else if (IsAsyncFunction(kind)) {
         ParseAsyncFunctionBody(inner_scope, &inner_body);
+      } else if (IsDerivedConstructor(kind)) {
+        ParseDerivedConstructorBody(&inner_body, closing_token);
+        {
+          ExpressionParsingScope expression_scope(impl());
+          inner_body.Add(factory()->NewReturnStatement(impl()->ThisExpression(),
+                                                       kNoSourcePosition));
+          expression_scope.ValidateExpression();
+        }
       } else {
         ParseStatementList(&inner_body, closing_token);
       }
 
-      if (IsDerivedConstructor(kind)) {
-        ExpressionParsingScope expression_scope(impl());
-        inner_body.Add(factory()->NewReturnStatement(impl()->ThisExpression(),
-                                                     kNoSourcePosition));
-        expression_scope.ValidateExpression();
-      }
       Expect(closing_token);
     }
   }
@@ -4712,6 +4730,40 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseV8Intrinsic() {
 
   return impl()->NewV8Intrinsic(name, args, pos);
 }
+template <typename Impl>
+void ParserBase<Impl>::ParseDerivedConstructorBody(StatementListT* body,
+                                                   Token::Value end_token) {
+  // Allocate a target stack to use for this set of source elements. This way,
+  // all scripts and functions get their own target stack thus avoiding illegal
+  // breaks and continues across functions.
+  TargetScopeT target_scope(this);
+  while (peek() != end_token) {
+    StatementT stat = impl()->NullStatement();
+    // We can elide some hole checks in a derived constructor with a top level
+    // 'super,' but only if 'this' has not been used before the super. For
+    // safety we also require hole checks if 'eval' was called before 'super.'
+    if (V8_UNLIKELY(peek() == Token::SUPER && PeekAhead() == Token::LPAREN) &&
+        !GetReceiverScope()->receiver()->is_used() &&
+        !GetReceiverScope()->inner_scope_calls_eval()) {
+      int pos = peek_position();
+      ExpressionT expr = ParseExpression();
+
+      // Check again to confirm the 'super' expression did not reference
+      // 'this' or use 'eval.'
+      DeclarationScope* receiver_scope = GetReceiverScope();
+      if (!receiver_scope->receiver()->is_used() &&
+          !receiver_scope->inner_scope_calls_eval()) {
+        receiver_scope->set_can_elide_this_hole_checks();
+      }
+      stat = factory()->NewExpressionStatement(expr, pos);
+    } else {
+      stat = ParseStatementListItem();
+    }
+    if (impl()->IsNull(stat)) return;
+    if (stat->IsEmptyStatement()) continue;
+    body->Add(stat);
+  }
+}
 
 template <typename Impl>
 void ParserBase<Impl>::ParseStatementList(StatementListT* body,

src/parsing/preparse-data.cc

@@ -26,6 +26,8 @@ using InnerScopeCallsEvalField =
     ScopeSloppyEvalCanExtendVarsField::Next<bool, 1>;
 using NeedsPrivateNameContextChainRecalcField =
     InnerScopeCallsEvalField::Next<bool, 1>;
+using CanElideThisHoleChecks =
+    NeedsPrivateNameContextChainRecalcField::Next<bool, 1>;
 
 using VariableMaybeAssignedField = BitField8<bool, 0, 1>;
 using VariableContextAllocatedField = VariableMaybeAssignedField::Next<bool, 1>;
@@ -354,7 +356,7 @@ void PreparseDataBuilder::SaveDataForScope(Scope* scope) {
   byte_data_.WriteUint8(scope->scope_type());
 #endif
 
-  uint8_t eval_and_private_recalc =
+  uint8_t scope_data_flags =
       ScopeSloppyEvalCanExtendVarsField::encode(
           scope->is_declaration_scope() &&
           scope->AsDeclarationScope()->sloppy_eval_can_extend_vars()) |
@@ -362,9 +364,12 @@ void PreparseDataBuilder::SaveDataForScope(Scope* scope) {
       NeedsPrivateNameContextChainRecalcField::encode(
           scope->is_function_scope() &&
           scope->AsDeclarationScope()
-              ->needs_private_name_context_chain_recalc());
+              ->needs_private_name_context_chain_recalc()) |
+      CanElideThisHoleChecks::encode(
+          scope->is_declaration_scope() &&
+          scope->AsDeclarationScope()->can_elide_this_hole_checks());
   byte_data_.Reserve(kUint8Size);
-  byte_data_.WriteUint8(eval_and_private_recalc);
+  byte_data_.WriteUint8(scope_data_flags);
 
   if (scope->is_function_scope()) {
     Variable* function = scope->AsDeclarationScope()->function_var();
@@ -605,17 +610,19 @@ void BaseConsumedPreparseData<Data>::RestoreDataForScope(Scope* scope) {
   DCHECK_EQ(scope_data_->ReadUint8(), scope->scope_type());
 
   CHECK(scope_data_->HasRemainingBytes(ByteData::kUint8Size));
-  uint32_t eval_and_private_recalc = scope_data_->ReadUint8();
-  if (ScopeSloppyEvalCanExtendVarsField::decode(eval_and_private_recalc)) {
+  uint32_t scope_data_flags = scope_data_->ReadUint8();
+  if (ScopeSloppyEvalCanExtendVarsField::decode(scope_data_flags)) {
     scope->RecordEvalCall();
   }
-  if (InnerScopeCallsEvalField::decode(eval_and_private_recalc)) {
+  if (InnerScopeCallsEvalField::decode(scope_data_flags)) {
     scope->RecordInnerScopeEvalCall();
   }
-  if (NeedsPrivateNameContextChainRecalcField::decode(
-          eval_and_private_recalc)) {
+  if (NeedsPrivateNameContextChainRecalcField::decode(scope_data_flags)) {
     scope->AsDeclarationScope()->RecordNeedsPrivateNameContextChainRecalc();
   }
+  if (CanElideThisHoleChecks::decode(scope_data_flags)) {
+    scope->AsDeclarationScope()->set_can_elide_this_hole_checks();
+  }
 
   if (scope->is_function_scope()) {
     Variable* function = scope->AsDeclarationScope()->function_var();

test/cctest/interpreter/bytecode_expectations/ClassAndSuperClass.golden

@@ -106,7 +106,7 @@ snippet: "
 "
 frame size: 6
 parameter count: 1
-bytecode array length: 40
+bytecode array length: 36
 bytecodes: [
                 B(Mov), R(closure), R(1),
   /*  113 E> */ B(StackCheck),
@@ -120,12 +120,9 @@ bytecodes: [
                 B(Ldar), R(this),
                 B(ThrowSuperAlreadyCalledIfNotHole),
                 B(Mov), R(5), R(this),
-  /*  128 S> */ B(Ldar), R(this),
-                B(ThrowSuperNotCalledIfHole),
-                B(LdaSmi), I8(2),
+  /*  128 S> */ B(LdaSmi), I8(2),
   /*  136 E> */ B(StaNamedProperty), R(this), U8(0), U8(2),
                 B(Ldar), R(this),
-                B(ThrowSuperNotCalledIfHole),
   /*  141 S> */ B(Return),
 ]
 constant pool: [
@@ -149,7 +146,7 @@ snippet: "
 "
 frame size: 5
 parameter count: 1
-bytecode array length: 36
+bytecode array length: 32
 bytecodes: [
                 B(Mov), R(closure), R(1),
   /*  112 E> */ B(StackCheck),
@@ -161,12 +158,9 @@ bytecodes: [
                 B(Ldar), R(this),
                 B(ThrowSuperAlreadyCalledIfNotHole),
                 B(Mov), R(4), R(this),
-  /*  126 S> */ B(Ldar), R(this),
-                B(ThrowSuperNotCalledIfHole),
-                B(LdaSmi), I8(2),
+  /*  126 S> */ B(LdaSmi), I8(2),
   /*  134 E> */ B(StaNamedProperty), R(this), U8(0), U8(2),
                 B(Ldar), R(this),
-                B(ThrowSuperNotCalledIfHole),
   /*  139 S> */ B(Return),
 ]
 constant pool: [

test/cctest/interpreter/bytecode_expectations/SuperCallAndSpread.golden

@@ -51,7 +51,7 @@ snippet: "
 "
 frame size: 9
 parameter count: 1
-bytecode array length: 40
+bytecode array length: 39
 bytecodes: [
                 B(CreateRestParameter),
                 B(Star), R(3),
@@ -70,7 +70,6 @@ bytecodes: [
                 B(ThrowSuperAlreadyCalledIfNotHole),
                 B(Mov), R(8), R(this),
                 B(Ldar), R(this),
-                B(ThrowSuperNotCalledIfHole),
   /*  159 S> */ B(Return),
 ]
 constant pool: [
@@ -93,7 +92,7 @@ snippet: "
 "
 frame size: 11
 parameter count: 1
-bytecode array length: 112
+bytecode array length: 111
 bytecodes: [
                 B(CreateRestParameter),
                 B(Star), R(3),
@@ -138,7 +137,6 @@ bytecodes: [
                 B(ThrowSuperAlreadyCalledIfNotHole),
                 B(Mov), R(9), R(this),
                 B(Ldar), R(this),
-                B(ThrowSuperNotCalledIfHole),
   /*  162 S> */ B(Return),
 ]
 constant pool: [

test/mjsunit/super_hole_check.mjs

@@ -0,0 +1,155 @@
+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+new class extends Object {
+  constructor() {
+    super(); this.foo = 1;
+  }
+}
+
+{
+  class Throws extends Object {
+    t = 1;
+    constructor(t = this.t) {
+      super();
+    }
+  }
+  assertThrows(() => new Throws, ReferenceError);
+}
+
+{
+  class Throws extends Object {
+    constructor() {
+      this.x = true;
+      super();
+    }
+  }
+  assertThrows(() => new Throws, ReferenceError);
+}
+
+{
+  class Throws extends Object {
+    constructor() {
+      super(this);
+    }
+  }
+  assertThrows(() => new Throws, ReferenceError);
+}
+
+{
+  class Throws extends Object {
+    constructor() {
+      super(eval("this"));
+    }
+  }
+  assertThrows(() => new Throws, ReferenceError);
+}
+
+{
+  class Throws extends Object {
+    constructor() {
+      super(eval("(() => this)()"));
+    }
+  }
+  assertThrows(() => new Throws, ReferenceError);
+}
+
+{
+  class Throws extends Object {
+    constructor(t = eval("this")) {
+      super();
+    }
+  }
+  assertThrows(() => new Throws, ReferenceError);
+}
+
+{
+  class Throws extends Object {
+    constructor() {
+      super();
+      super();
+    }
+  }
+  assertThrows(() => new Throws, ReferenceError);
+}
+
+{
+  class Throws extends Object {
+    constructor() {
+      if (false) { super(); }
+      this.x = true;
+    }
+  }
+  assertThrows(() => new Throws, ReferenceError);
+}
+
+{
+  class Throws extends Object {
+    constructor() {
+      super((4 * this.t));
+    }
+  }
+  assertThrows(() => new Throws, ReferenceError);
+}
+
+{
+  class Throws extends Object {
+    constructor() {
+      super((() => this)());
+    }
+  }
+  assertThrows(() => new Throws(), ReferenceError);
+}
+
+{
+  class Throws extends Object {
+    constructor() {
+      super((()=>{ var x = ()=> this; return x(); })())
+    }
+  }
+  assertThrows(() => new Throws(), ReferenceError);
+}
+
+{
+  class C extends null {
+    constructor() {
+      super();
+    }
+  }
+  assertThrows(() => new C(), TypeError);
+}
+
+{
+  class C extends Object {
+    constructor() {
+      super();
+      (() => {
+        this;
+      })();
+    }
+  }
+  new C();
+}
+
+{
+  var count = 0;
+
+  class A {
+    constructor() {
+      count++;
+    }
+    increment() {
+      count++;
+    }
+  }
+
+  class B extends A {
+    constructor() {
+      super();
+      (_ => super.increment())();
+    }
+  }
+  new B();
+  assertEquals(count, 2);
+}