[refactoring] Split CodeAssemblerState out of CodeAssembler

src/compiler/code-assembler.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_COMPILER_CODE_ASSEMBLER_H_
 #define V8_COMPILER_CODE_ASSEMBLER_H_
 
 #include <map>
 #include <memory>
 
@@ skipping 155 matching lines @@
 // V8 components that need to generate low-level code using this interface
 // should include this header--and this header only--from the compiler directory
 // (this is actually enforced). Since all interesting data structures are
 // forward declared, it's not possible for clients to peek inside the compiler
 // internals.
 //
 // In addition to providing isolation between TurboFan and code generation
 // clients, CodeAssembler also provides an abstraction for creating variables
 // and enhanced Label functionality to merge variable values along paths where
 // they have differing values, including loops.
+//
+// The CodeAssembler itself is stateless (and instances are expected to be
+// temporary-scoped and short-lived); all its state is encapsulated into
+// a CodeAssemblerState instance.
+
+class CodeAssemblerState;

[Michael Starzinger, 2016/11/15 11:45:20]

nit: Please move this forward declaration up to the other ones at the top of
this file.

[Jakob Kummerow, 2016/11/15 13:40:32]

Done.

+
 class V8_EXPORT_PRIVATE CodeAssembler {
  public:
-  // Create with CallStub linkage.
-  // |result_size| specifies the number of results returned by the stub.
-  // TODO(rmcilroy): move result_size to the CallInterfaceDescriptor.
-  CodeAssembler(Isolate* isolate, Zone* zone,
-                const CallInterfaceDescriptor& descriptor, Code::Flags flags,
-                const char* name, size_t result_size = 1);
-
-  // Create with JSCall linkage.
-  CodeAssembler(Isolate* isolate, Zone* zone, int parameter_count,
-                Code::Flags flags, const char* name);
+  explicit CodeAssembler(CodeAssemblerState* state) : state_(state) {}
 
   virtual ~CodeAssembler();
 
   Handle<Code> GenerateCode();
 
   bool Is64() const;
   bool IsFloat64RoundUpSupported() const;
   bool IsFloat64RoundDownSupported() const;
   bool IsFloat64RoundTruncateSupported() const;
 
   class Label;
   class Variable {
    public:
     explicit Variable(CodeAssembler* assembler, MachineRepresentation rep);
     ~Variable();
     void Bind(Node* value);
     Node* value() const;
     MachineRepresentation rep() const;
     bool IsBound() const;
 
    private:
     friend class CodeAssembler;
+    friend class CodeAssemblerState;
     class Impl;
     Impl* impl_;
     CodeAssembler* assembler_;
   };
 
   typedef ZoneList<Variable*> VariableList;
 
   // ===========================================================================
   // Base Assembler
   // ===========================================================================
@@ skipping 244 matching lines @@
   Factory* factory() const;
   Isolate* isolate() const;
   Zone* zone() const;
 
  protected:
   // Enables subclasses to perform operations before and after a call.
   virtual void CallPrologue();
   virtual void CallEpilogue();
 
  private:
-  CodeAssembler(Isolate* isolate, Zone* zone, CallDescriptor* call_descriptor,
-                Code::Flags flags, const char* name);
-
   Node* CallN(CallDescriptor* descriptor, Node* code_target, Node** args);
   Node* TailCallN(CallDescriptor* descriptor, Node* code_target, Node** args);
 
-  std::unique_ptr<RawMachineAssembler> raw_assembler_;
-  Code::Flags flags_;
-  const char* name_;
-  bool code_generated_;
-  ZoneSet<Variable::Impl*> variables_;
+  RawMachineAssembler* raw_assembler() const;
+
+  CodeAssemblerState* state_;
 
   DISALLOW_COPY_AND_ASSIGN(CodeAssembler);
 };
 
 class CodeAssembler::Label {
  public:
   enum Type { kDeferred, kNonDeferred };
 
   explicit Label(
       CodeAssembler* assembler,
@@ skipping 21 matching lines @@
   CodeAssembler* assembler_;
   RawMachineLabel* label_;
   // Map of variables that need to be merged to their phi nodes (or placeholders
   // for those phis).
   std::map<Variable::Impl*, Node*> variable_phis_;
   // Map of variables to the list of value nodes that have been added from each
   // merge path in their order of merging.
   std::map<Variable::Impl*, std::vector<Node*>> variable_merges_;
 };
 
+class V8_EXPORT_PRIVATE CodeAssemblerState {

[Michael Starzinger, 2016/11/15 11:45:20]

nit: Can we make this final?

[Igor Sheludko, 2016/11/15 13:06:35]

Thinking out loud: we could even hide this class inside .cc file if we add
static methods that create CodeAssemblerState object to CodeAssembler class.

[Jakob Kummerow, 2016/11/15 13:40:32]

We could do that. But then we can't heap-allocate them any more. Do we want
that?

[Jakob Kummerow, 2016/11/15 13:40:32]

I'd love to; but see comment on unit test.

[Igor Sheludko, 2016/11/15 14:02:53]

Ah, I missed the DISALLOW_COPY_AND_ASSIGN thing.

+ public:
+  // Create with CallStub linkage.
+  // |result_size| specifies the number of results returned by the stub.
+  // TODO(rmcilroy): move result_size to the CallInterfaceDescriptor.
+  CodeAssemblerState(Isolate* isolate, Zone* zone,
+                     const CallInterfaceDescriptor& descriptor,
+                     Code::Flags flags, const char* name,
+                     size_t result_size = 1);
+
+  // Create with JSCall linkage.
+  CodeAssemblerState(Isolate* isolate, Zone* zone, int parameter_count,
+                     Code::Flags flags, const char* name);
+
+  ~CodeAssemblerState();
+
+  Handle<Code> GenerateCode();

[Michael Starzinger, 2016/11/15 11:45:20]

Can we move this back to the {CodeAssembler} class? That way this class is pure
container class without logic. The only way the state of this class changes is
by using the {CodeAssmelber} to mutate it. WDYT?

[Igor Sheludko, 2016/11/15 13:06:35]

If we do that then we will not be able to generate builtins code from the state
as we do in BuildWithCodeStubAssembler*().

[Jakob Kummerow, 2016/11/15 13:40:32]

Summarizing offline discussion:
- The argument in favor of moving this back is so that the State is just a data
container with no logic at all. That design preserves maximum freedom from the
compiler:: side to change under-the-hood implementation details.
- The argument in favor of keeping it on the State object is that the various
*Assembler classes are responsible for accumulating things in the State
container. Taking the accumulated stuff out of the container is a separate use
case; having to instantiate a CodeAssembler just to be able to do that doesn't
seem to add much value.

Compromise idea: make it a static function!

[Igor Sheludko, 2016/11/15 14:02:53]

Awesome!

+
+ private:
+  friend class CodeAssembler;
+
+  CodeAssemblerState(Isolate* isolate, Zone* zone,
+                     CallDescriptor* call_descriptor, Code::Flags flags,
+                     const char* name);
+
+  std::unique_ptr<RawMachineAssembler> raw_assembler_;
+  Code::Flags flags_;
+  const char* name_;
+  bool code_generated_;
+  ZoneSet<CodeAssembler::Variable::Impl*> variables_;
+
+  DISALLOW_COPY_AND_ASSIGN(CodeAssemblerState);
+};
+
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_COMPILER_CODE_ASSEMBLER_H_