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

src/interpreter/bytecode-generator.h

Index: src/interpreter/bytecode-generator.h
diff --git a/src/interpreter/bytecode-generator.h b/src/interpreter/bytecode-generator.h
index 0447aa8f4522e161c3502e95710f5e02364ddeb2..e8ffd5998f1d1508a2716e08c3639ad023d2b653 100644
--- a/src/interpreter/bytecode-generator.h
+++ b/src/interpreter/bytecode-generator.h
@@ -94,16 +94,13 @@ class BytecodeGenerator final : public AstVisitor<BytecodeGenerator> {
   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();
@@ -111,7 +108,7 @@ class BytecodeGenerator final : public AstVisitor<BytecodeGenerator> {
   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