[ignition] Eliminate hole checks where statically possible for loads and stores

src/interpreter/bytecode-generator.h

 // Copyright 2015 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.
 
 #ifndef V8_INTERPRETER_BYTECODE_GENERATOR_H_
 #define V8_INTERPRETER_BYTECODE_GENERATOR_H_
 
 #include "src/ast/ast.h"
 #include "src/interpreter/bytecode-array-builder.h"
 #include "src/interpreter/bytecode-label.h"
@@ skipping 76 matching lines @@
   // Visit a named super property load. The optional
   // |opt_receiver_out| register will have the receiver stored to it
   // if it's a valid register. The loaded value is placed in the
   // accumulator.
   void VisitNamedSuperPropertyLoad(Property* property,
                                    Register opt_receiver_out);
 
   void VisitPropertyLoad(Register obj, Property* expr);
   void VisitPropertyLoadForAccumulator(Register obj, Property* expr);
 
-  void VisitVariableLoad(Variable* variable, FeedbackVectorSlot slot,
+  void BuildVariableLoad(VariableProxy* proxy,

[Michael Starzinger, 2016/10/17 14:15:35]

nit: Not a huge fan of having to pass in the variable proxy for loading. I know
boolean flags are also ugly, but in this case I would actually prefer
BuildVariableLoad(Variable*, FeedbackVectorSlot, bool) as a signature. Similar
for the other methods below. Especially in light of BuildVariableAssignment
being used in places where no valid proxy is available. WDYT?

[adamk, 2016/10/19 11:09:43]

Do you mean BuildVariableAssignment? I agree there's ugliness in having to pass
both a variable and a proxy there, and a bool isn't really any worse (and I'm
happy to replace that fourth argument). But for BuildVariableLoad passing a
proxy seems strictly simpler. Why are you not a fan of passing a proxy there?

[Michael Starzinger, 2016/10/19 12:32:41]

Yes, my comment was inspired by the "ugliness" in BuildVariableAssignment. And
then I extended my train of thought to all three methods. My goal was to keep
them in sync so that they all either operate on the {Variable} level or on the
{VariableProxy} level, instead of a mixed API where loading works on
{VariableProxy} and assignment works on {Variable}.

Not too strong of an opinion and I can also live with how it is now. So feel
free to ignore my nit here.

[adamk, 2016/10/19 13:34:23]

I can see the argument for consistency, but it does make the callers of
BuildVariableLoad rather verbose. See what you think of patchset 8.

[Michael Starzinger, 2016/10/19 15:41:52]

I would be fine with patch set #8.

                          TypeofMode typeof_mode = NOT_INSIDE_TYPEOF);
-  void VisitVariableLoadForAccumulatorValue(
-      Variable* variable, FeedbackVectorSlot slot,
-      TypeofMode typeof_mode = NOT_INSIDE_TYPEOF);
-  MUST_USE_RESULT Register
-  VisitVariableLoadForRegisterValue(Variable* variable, FeedbackVectorSlot slot,
-                                    TypeofMode typeof_mode = NOT_INSIDE_TYPEOF);
-  void VisitVariableAssignment(Variable* variable, Token::Value op,
-                               FeedbackVectorSlot slot);
+  void BuildVariableLoadForAccumulatorValue(
+      VariableProxy* proxy, TypeofMode typeof_mode = NOT_INSIDE_TYPEOF);
+  void BuildVariableAssignment(Variable* variable, Token::Value op,
+                               FeedbackVectorSlot slot,
+                               VariableProxy* proxy = nullptr);
 
   void BuildReturn();
   void BuildReThrow();
   void BuildAbort(BailoutReason bailout_reason);
   void BuildThrowIfHole(Handle<String> name);
   void BuildThrowIfNotHole(Handle<String> name);
   void BuildThrowReferenceError(Handle<String> name);
-  void BuildHoleCheckForVariableLoad(Variable* variable);
+  void BuildHoleCheckForVariableLoad(VariableProxy* proxy);
   void BuildHoleCheckForVariableAssignment(Variable* variable, Token::Value op);
 
   // Build jump to targets[value], where
   // start_index <= value < start_index + size.
   void BuildIndexedJump(Register value, size_t start_index, size_t size,
                         ZoneVector<BytecodeLabel>& targets);
 
   void BuildNewLocalActivationContext();
   void BuildLocalActivationContextInitialization();
   void BuildNewLocalBlockContext(Scope* scope);
@@ skipping 92 matching lines @@
 
   Handle<Name> home_object_symbol_;
   Handle<Name> prototype_string_;
 };
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_INTERPRETER_BYTECODE_GENERATOR_H_