Eliminate temporary locals for some expressions

runtime/vm/ast.h

Index: runtime/vm/ast.h
===================================================================
--- runtime/vm/ast.h	(revision 23243)
+++ runtime/vm/ast.h	(working copy)
@@ -55,6 +55,7 @@
   V(LoadIndexedNode, "load indexed")                                           \
   V(StoreIndexedNode, "store indexed")                                         \
   V(SequenceNode, "seq")                                                       \
+  V(LetNode, "let")                                                            \
   V(CommaNode, "comma")                                                        \
   V(CatchClauseNode, "catch clause block")                                     \
   V(TryCatchNode, "try catch block")                                           \
@@ -293,24 +294,51 @@
 };
 
 
+class LetNode : public AstNode {
+ public:
+  LetNode(intptr_t token_pos, intptr_t num_temps);
+
+  LocalVariable* temp(intptr_t i) const { return vars_[i]; }
+  const GrowableArray<AstNode*>& temp_expressions() const {

[Kevin Millikin (Google), 2013/05/29 11:28:23]

'temp_expression' sounds like the expression is temporary.  I would use
something more like 'initializer'.

[Florian Schneider, 2013/05/30 09:29:31]

Done.

+    return temp_expressions_;
+  }
+  void SetTempExpression(intptr_t i, AstNode* node) {

[hausner, 2013/05/28 17:00:47]

We typically use the ending -At for setters that are indexed:
SetTempExpressionAt(i, node). 

[Florian Schneider, 2013/05/30 09:29:31]

Redesigned to add expressions instead of preallocating and setting.

+    temp_expressions_[i] = node;
+  }
+
+  intptr_t num_temps() const {
+    return vars_.length();
+  }
+
+  AstNode* body() const { return body_; }
+  void set_body(AstNode* node) { body_ = node; }
+
+  void VisitChildren(AstNodeVisitor* visitor) const;
+
+  DECLARE_COMMON_NODE_FUNCTIONS(LetNode);
+
+ private:
+  GrowableArray<LocalVariable*> vars_;
+  GrowableArray<AstNode*> temp_expressions_;
+  AstNode* body_;

[Kevin Millikin (Google), 2013/05/29 11:28:23]

I think you should: (1) allow a list of AstNode* in the body instead of just
one, (2) eliminate CommaNode and replace all occurrences with a LetNode with 0
bindings.

It should just work in the graph builder and compiler (provided __ Drop(0) has
special handling), except for some wartiness about LetNode in effect contexts
(which I think should be fixed anyway).

[Florian Schneider, 2013/05/30 09:29:31]

Yes, I'll do that as a separate CL and added a TODO to replace CommaNode.
LetNode works with zero temp vars, the code generated should be the same. e.g.
in ParseCascades, we can simply use a LetNode instead of
LetNode+SequenceNode+CommaNode

+
+  DISALLOW_COPY_AND_ASSIGN(LetNode);
+};
+
+
 class ArrayNode : public AstNode {
  public:
-  ArrayNode(intptr_t token_pos,
-            const AbstractType& type,
-            const LocalVariable& temp)
+  ArrayNode(intptr_t token_pos, const AbstractType& type)
       : AstNode(token_pos),
         type_(type),
-        temp_local_(temp),
         elements_() {
     CheckFields();
   }
   ArrayNode(intptr_t token_pos,
             const AbstractType& type,
-            const LocalVariable& temp,
             const GrowableArray<AstNode*>& elements)
       : AstNode(token_pos),
         type_(type),
-        temp_local_(temp),
         elements_(elements.length()) {
     CheckFields();
     for (intptr_t i = 0; i < elements.length(); i++) {
@@ -330,13 +358,10 @@
 
   const AbstractType& type() const { return type_; }
 
-  const LocalVariable& temp_local() const { return temp_local_; }
-
   DECLARE_COMMON_NODE_FUNCTIONS(ArrayNode);
 
  private:
   const AbstractType& type_;
-  const LocalVariable& temp_local_;  // Store allocated array while filling it.
   GrowableArray<AstNode*> elements_;
 
   void CheckFields() {
@@ -1507,25 +1532,19 @@
 // instantiator is the first parameter of this factory, which is already a
 // type argument vector. This case is identified by a null and unneeded
 // instantiator_class.
-//
-// A temporary local is needed to hold the allocated value while the
-// constructor is being called.
 class ConstructorCallNode : public AstNode {
  public:
   ConstructorCallNode(intptr_t token_pos,
                       const AbstractTypeArguments& type_arguments,
                       const Function& constructor,
-                      ArgumentListNode* arguments,
-                      const LocalVariable* allocated_object_var)
+                      ArgumentListNode* arguments)
       : AstNode(token_pos),
         type_arguments_(type_arguments),
         constructor_(constructor),
-        arguments_(arguments),
-        allocated_object_var_(*allocated_object_var) {
+        arguments_(arguments) {
     ASSERT(type_arguments_.IsZoneHandle());
     ASSERT(constructor_.IsZoneHandle());
     ASSERT(arguments_ != NULL);
-    ASSERT(allocated_object_var != NULL);
   }
 
   const AbstractTypeArguments& type_arguments() const {
@@ -1533,9 +1552,6 @@
   }
   const Function& constructor() const { return constructor_; }
   ArgumentListNode* arguments() const { return arguments_; }
-  const LocalVariable& allocated_object_var() const {
-    return allocated_object_var_;
-  }
 
   virtual void VisitChildren(AstNodeVisitor* visitor) const {
     arguments()->Visit(visitor);
@@ -1547,7 +1563,6 @@
   const AbstractTypeArguments& type_arguments_;
   const Function& constructor_;
   ArgumentListNode* arguments_;
-  const LocalVariable& allocated_object_var_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(ConstructorCallNode);
 };