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

This reverts commit 10883f56.

Reason for revert: Causes bytecode mismatch

Bug:chromium:1020538, chromium:1021457

Original change's description:
> [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}

TBR=rmcilroy@chromium.org,leszeks@chromium.org,verwaest@chromium.org,joshualitt@chromium.org

Bug: v8:7427
Change-Id: Ib4369a3560e929692585c4546435684deae5ee9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1899163
Commit-Queue: Joshua Litt <joshualitt@chromium.org>
Reviewed-by: Joshua Litt <joshualitt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64789}

src/ast/scopes.cc

@@ -215,9 +215,6 @@ 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,
@@ -251,7 +248,6 @@ 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

@@ -1109,13 +1109,11 @@ 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_; }
-
-  bool can_elide_this_hole_checks() const {
-    return can_elide_this_hole_checks_;
+  void UsesThis() {
+    set_has_this_reference();
+    GetReceiverScope()->receiver()->ForceContextAllocation();
   }
 
-  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_;
   }
@@ -1165,7 +1163,6 @@ 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

@@ -2144,9 +2144,6 @@ void ScopeInfo::ScopeInfoPrint(std::ostream& os) {  // NOLINT
   if (HasContextExtensionSlot()) {
     os << "\n - has context extension slot";
   }
-  if (CanElideThisHoleChecks()) {
-    os << "\n - can elide this hole checks";
-  }
 
   if (HasPositionInfo()) {
     os << "\n - start position: " << StartPosition();

src/interpreter/bytecode-generator.cc

@@ -5626,9 +5626,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()) &&
-       !receiver_scope->can_elide_this_hole_checks())
+      IsDerivedConstructor(receiver_scope->function_kind())
           ? HoleCheckMode::kRequired
           : HoleCheckMode::kElided;
   BuildVariableLoad(var, hole_check_mode);

src/objects/scope-info.cc

@@ -173,7 +173,6 @@ 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();
@@ -184,8 +183,6 @@ 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.
@@ -210,7 +207,6 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope,
             scope->ForceContextForLanguageMode()) |
         PrivateNameLookupSkipsOuterClassField::encode(
             scope->private_name_lookup_skips_outer_class()) |
-        CanElideThisHoleChecksField::encode(can_elide_this_hole_checks) |
         HasContextExtensionSlotField::encode(scope->HasContextExtensionSlot());
     scope_info.SetFlags(flags);
 
@@ -398,7 +394,6 @@ Handle<ScopeInfo> ScopeInfo::CreateForWithScope(
       IsDebugEvaluateScopeField::encode(false) |
       ForceContextAllocationField::encode(false) |
       PrivateNameLookupSkipsOuterClassField::encode(false) |
-      CanElideThisHoleChecksField::encode(false) |
       HasContextExtensionSlotField::encode(true);
   scope_info->SetFlags(flags);
 
@@ -476,7 +471,6 @@ Handle<ScopeInfo> ScopeInfo::CreateForBootstrapping(Isolate* isolate,
               IsDebugEvaluateScopeField::encode(false) |
               ForceContextAllocationField::encode(false) |
               PrivateNameLookupSkipsOuterClassField::encode(false) |
-              CanElideThisHoleChecksField::encode(false) |
               HasContextExtensionSlotField::encode(is_native_context);
   scope_info->SetFlags(flags);
   scope_info->SetParameterCount(parameter_count);
@@ -674,11 +668,6 @@ 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,9 +53,6 @@ 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 header for every context.
@@ -270,10 +267,8 @@ class ScopeInfo : public FixedArray {
   using ForceContextAllocationField = IsDebugEvaluateScopeField::Next<bool, 1>;
   using PrivateNameLookupSkipsOuterClassField =
       ForceContextAllocationField::Next<bool, 1>;
-  using CanElideThisHoleChecksField =
-      PrivateNameLookupSkipsOuterClassField::Next<bool, 1>;
   using HasContextExtensionSlotField =
-      CanElideThisHoleChecksField::Next<bool, 1>;
+      PrivateNameLookupSkipsOuterClassField::Next<bool, 1>;
 
   STATIC_ASSERT(kLastFunctionKind <= FunctionKindField::kMax);
 

src/parsing/expression-scope.h

@@ -145,16 +145,6 @@ 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?
@@ -780,12 +770,7 @@ class ArrowHeadParsingScope : public ExpressionParsingScope<Types> {
     }
 #endif  // DEBUG
 
-    if (uses_this_) {
-      result->set_has_this_reference();
-    }
-    if (uses_this_ || calls_super_) {
-      result->GetReceiverScope()->receiver()->ForceContextAllocation();
-    }
+    if (uses_this_) result->UsesThis();
     return result;
   }
 
@@ -798,7 +783,6 @@ 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 {
@@ -811,7 +795,6 @@ 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

@@ -1005,21 +1005,6 @@ 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
@@ -1113,8 +1098,6 @@ 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,
@@ -3504,7 +3487,8 @@ 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.
-      CallsSuper();
+      expression_scope()->RecordThisUse();
+      UseThis();
       return impl()->NewSuperCallReference(pos);
     }
   }
@@ -4087,18 +4071,16 @@ 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);
-        {
+      } else {
+        ParseStatementList(&inner_body, closing_token);
+      }
+
+      if (IsDerivedConstructor(kind)) {
         ExpressionParsingScope expression_scope(impl());
         inner_body.Add(factory()->NewReturnStatement(impl()->ThisExpression(),
                                                      kNoSourcePosition));
         expression_scope.ValidateExpression();
       }
-      } else {
-        ParseStatementList(&inner_body, closing_token);
-      }
-
       Expect(closing_token);
     }
   }
@@ -4768,40 +4750,6 @@ 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,9 +26,8 @@ using InnerScopeCallsEvalField =
     ScopeSloppyEvalCanExtendVarsField::Next<bool, 1>;
 using NeedsPrivateNameContextChainRecalcField =
     InnerScopeCallsEvalField::Next<bool, 1>;
-using CanElideThisHoleChecks =
+using ShouldSaveClassVariableIndexField =
     NeedsPrivateNameContextChainRecalcField::Next<bool, 1>;
-using ShouldSaveClassVariableIndexField = CanElideThisHoleChecks::Next<bool, 1>;
 
 using VariableMaybeAssignedField = BitField8<bool, 0, 1>;
 using VariableContextAllocatedField = VariableMaybeAssignedField::Next<bool, 1>;
@@ -357,7 +356,7 @@ void PreparseDataBuilder::SaveDataForScope(Scope* scope) {
   byte_data_.WriteUint8(scope->scope_type());
 #endif
 
-  uint8_t scope_data_flags =
+  uint8_t eval_and_private_recalc =
       ScopeSloppyEvalCanExtendVarsField::encode(
           scope->is_declaration_scope() &&
           scope->AsDeclarationScope()->sloppy_eval_can_extend_vars()) |
@@ -366,14 +365,11 @@ void PreparseDataBuilder::SaveDataForScope(Scope* scope) {
           scope->is_function_scope() &&
           scope->AsDeclarationScope()
               ->needs_private_name_context_chain_recalc()) |
-      CanElideThisHoleChecks::encode(
-          scope->is_declaration_scope() &&
-          scope->AsDeclarationScope()->can_elide_this_hole_checks()) |
       ShouldSaveClassVariableIndexField::encode(
           scope->is_class_scope() &&
           scope->AsClassScope()->should_save_class_variable_index());
   byte_data_.Reserve(kUint8Size);
-  byte_data_.WriteUint8(scope_data_flags);
+  byte_data_.WriteUint8(eval_and_private_recalc);
 
   if (scope->is_function_scope()) {
     Variable* function = scope->AsDeclarationScope()->function_var();
@@ -625,9 +621,6 @@ void BaseConsumedPreparseData<Data>::RestoreDataForScope(
   if (NeedsPrivateNameContextChainRecalcField::decode(scope_data_flags)) {
     scope->AsDeclarationScope()->RecordNeedsPrivateNameContextChainRecalc();
   }
-  if (CanElideThisHoleChecks::decode(scope_data_flags)) {
-    scope->AsDeclarationScope()->set_can_elide_this_hole_checks();
-  }
   if (ShouldSaveClassVariableIndexField::decode(scope_data_flags)) {
     Variable* var;
     // An anonymous class whose class variable needs to be saved do not

test/cctest/interpreter/bytecode_expectations/ClassAndSuperClass.golden

@@ -106,7 +106,7 @@ snippet: "
 "
 frame size: 6
 parameter count: 1
-bytecode array length: 36
+bytecode array length: 40
 bytecodes: [
                 B(Mov), R(closure), R(1),
   /*  113 E> */ B(StackCheck),
@@ -120,9 +120,12 @@ bytecodes: [
                 B(Ldar), R(this),
                 B(ThrowSuperAlreadyCalledIfNotHole),
                 B(Mov), R(5), R(this),
-  /*  128 S> */ B(LdaSmi), I8(2),
+  /*  128 S> */ B(Ldar), R(this),
+                B(ThrowSuperNotCalledIfHole),
+                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: [
@@ -146,7 +149,7 @@ snippet: "
 "
 frame size: 5
 parameter count: 1
-bytecode array length: 32
+bytecode array length: 36
 bytecodes: [
                 B(Mov), R(closure), R(1),
   /*  112 E> */ B(StackCheck),
@@ -158,9 +161,12 @@ bytecodes: [
                 B(Ldar), R(this),
                 B(ThrowSuperAlreadyCalledIfNotHole),
                 B(Mov), R(4), R(this),
-  /*  126 S> */ B(LdaSmi), I8(2),
+  /*  126 S> */ B(Ldar), R(this),
+                B(ThrowSuperNotCalledIfHole),
+                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: 39
+bytecode array length: 40
 bytecodes: [
                 B(CreateRestParameter),
                 B(Star), R(3),
@@ -70,6 +70,7 @@ bytecodes: [
                 B(ThrowSuperAlreadyCalledIfNotHole),
                 B(Mov), R(8), R(this),
                 B(Ldar), R(this),
+                B(ThrowSuperNotCalledIfHole),
   /*  159 S> */ B(Return),
 ]
 constant pool: [
@@ -92,7 +93,7 @@ snippet: "
 "
 frame size: 11
 parameter count: 1
-bytecode array length: 111
+bytecode array length: 112
 bytecodes: [
                 B(CreateRestParameter),
                 B(Star), R(3),
@@ -137,6 +138,7 @@ 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

-// 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);
-}