[torque] Compile and error check all inlined macros

Previously, macros that returned true for "ShouldBeInlined" were only
compiled if they were called, which made it impossible to
type/semantic check all Torque code (e.g. newly added methods to
structs). One might argue that all code should be tested (and thus
through inlining compiled), but for prototyping, the skipped
compilations were definitely annoying.

As part of this change, added a ShouldGenerateExternalCode method to
declarables (by default returns !ShouldBeInlined) that makes it
possible to suppresses C++ code generation for any method. To
support this at the lowest level, a NullOStream classes is added as
part of this patch.

Finally, added support for generating C++ for passing structs as label
parameters to run previously inlined methods through the
implementation-visitor for non-inlined compilation.

Bug: v8:7793
Change-Id: I8ce23382e12ddc25f46222c25729c82433040a73
Reviewed-on: https://chromium-review.googlesource.com/c/1434378
Commit-Queue: Daniel Clifford <danno@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59134}

src/builtins/base.tq

@@ -82,7 +82,7 @@ type Constructor extends JSReceiver generates 'TNode<JSReceiver>';
 type JSProxy extends JSReceiver generates 'TNode<JSProxy>';
 
 class JSObject extends JSReceiver {
-  constructor(map: Map, properties: FixedArrayBase, elements: FixedArrayBase) {
+  constructor(map: Tagged, properties: Object, elements: FixedArrayBase) {
     super(map, properties);
     this.elements = elements;
   }

src/torque/declarable.h

@@ -254,6 +254,7 @@ class Callable : public Scope {
   base::Optional<Statement*> body() const { return body_; }
   bool IsExternal() const { return !body_.has_value(); }
   virtual bool ShouldBeInlined() const { return false; }
+  virtual bool ShouldGenerateExternalCode() const { return !ShouldBeInlined(); }
   bool IsConstructor() const { return readable_name_ == kConstructMethodName; }
 
  protected:

src/torque/implementation-visitor.cc

@@ -295,11 +295,6 @@ VisitResult ImplementationVisitor::InlineMacro(
 }
 
 void ImplementationVisitor::VisitMacroCommon(Macro* macro) {
-  // Do not generate code for inlined macros.
-  if (macro->ShouldBeInlined()) {
-    return;
-  }
-
   CurrentCallable::Scope current_callable(macro);
   const Signature& signature = macro->signature();
   const Type* return_type = macro->signature().return_type;
@@ -307,10 +302,6 @@ void ImplementationVisitor::VisitMacroCommon(Macro* macro) {
   bool has_return_value =
       can_return && return_type != TypeOracle::GetVoidType();
 
-  // Struct methods should never generate code, they should always be inlined
-  DCHECK(!macro->IsMethod() ||
-         Method::cast(macro)->aggregate_type()->IsClassType());
-
   header_out() << "  ";
   GenerateMacroFunctionDeclaration(header_out(), "", macro);
   header_out() << ";\n";
@@ -327,14 +318,23 @@ void ImplementationVisitor::VisitMacroCommon(Macro* macro) {
   base::Optional<LocationReference> this_reference;
   if (Method* method = Method::DynamicCast(macro)) {
     const Type* this_type = method->aggregate_type();
-    DCHECK(this_type->IsClassType());
-    lowered_parameter_types.Push(this_type);
-    lowered_parameters.Push(ExternalParameterName(kThisParameterName));
-    VisitResult this_result =
-        VisitResult(this_type, lowered_parameters.TopRange(1));
-    // Mark the this as a temporary to prevent assignment to it.
+    LowerParameter(this_type, ExternalParameterName(kThisParameterName),
+                   &lowered_parameters);
+    StackRange range = lowered_parameter_types.PushMany(LowerType(this_type));
+    VisitResult this_result = VisitResult(this_type, range);
+    // For classes, mark 'this' as a temporary to prevent assignment to it.
+    // Note that using a VariableAccess for non-class types is technically
+    // incorrect because changes to the 'this' variable do not get reflected
+    // to the caller. Therefore struct methods should always be inlined and a
+    // C++ version should never be generated, since it would be incorrect.
+    // However, in order to be able to type- and semantics-check even unused
+    // struct methods, set the this_reference to be the local variable copy of
+    // the passed-in this, which allows the visitor to at least find and report
+    // errors.
     this_reference =
-        LocationReference::Temporary(this_result, "this parameter");
+        (this_type->IsClassType())
+            ? LocationReference::Temporary(this_result, "this parameter")
+            : LocationReference::VariableAccess(this_result);
   }
 
   for (size_t i = 0; i < macro->signature().parameter_names.size(); ++i) {
@@ -378,8 +378,9 @@ void ImplementationVisitor::VisitMacroCommon(Macro* macro) {
     assembler().Bind(label_block);
     std::vector<std::string> label_parameter_variables;
     for (size_t i = 0; i < label_info.types.size(); ++i) {
-      label_parameter_variables.push_back(
-          ExternalLabelParameterName(label_info.name, i));
+      LowerLabelParameter(label_info.types[i],
+                          ExternalLabelParameterName(label_info.name, i),
+                          &label_parameter_variables);
     }
     assembler().Emit(GotoExternalInstruction{ExternalLabelName(label_info.name),
                                              label_parameter_variables});
@@ -1423,9 +1424,14 @@ void ImplementationVisitor::GenerateFunctionDeclaration(
     o << "compiler::CodeAssemblerLabel* " << ExternalLabelName(label_info.name);
     size_t i = 0;
     for (const Type* type : label_info.types) {
-      std::string generated_type_name("compiler::TypedCodeAssemblerVariable<");
+      std::string generated_type_name;
+      if (type->IsStructType()) {
+        generated_type_name = "\n#error no structs allowed in labels\n";
+      } else {
+        generated_type_name = "compiler::TypedCodeAssemblerVariable<";
         generated_type_name += type->GetGeneratedTNodeTypeName();
         generated_type_name += ">*";
+      }
       o << ", ";
       o << generated_type_name << " "
         << ExternalLabelParameterName(label_info.name, i);
@@ -2326,6 +2332,21 @@ StackRange ImplementationVisitor::LowerParameter(
   }
 }
 
+void ImplementationVisitor::LowerLabelParameter(
+    const Type* type, const std::string& parameter_name,
+    std::vector<std::string>* lowered_parameters) {
+  if (const StructType* struct_type = StructType::DynamicCast(type)) {
+    for (auto& field : struct_type->fields()) {
+      LowerLabelParameter(
+          field.name_and_type.type,
+          "&((*" + parameter_name + ")." + field.name_and_type.name + ")",
+          lowered_parameters);
+    }
+  } else {
+    lowered_parameters->push_back(parameter_name);
+  }
+}
+
 std::string ImplementationVisitor::ExternalLabelName(
     const std::string& label_name) {
   return "label_" + label_name;
@@ -2393,6 +2414,7 @@ void ImplementationVisitor::GenerateCatchBlock(
 }
 
 void ImplementationVisitor::VisitAllDeclarables() {
+  CurrentCallable::Scope current_callable(nullptr);
   const std::vector<std::unique_ptr<Declarable>>& all_declarables =
       GlobalContext::AllDeclarables();
   // This has to be an index-based loop because all_declarables can be extended

src/torque/implementation-visitor.h

@@ -478,15 +478,30 @@ class ImplementationVisitor : public FileVisitor {
   StackRange LowerParameter(const Type* type, const std::string& parameter_name,
                             Stack<std::string>* lowered_parameters);
 
+  void LowerLabelParameter(const Type* type, const std::string& parameter_name,
+                           std::vector<std::string>* lowered_parameters);
+
   std::string ExternalLabelName(const std::string& label_name);
   std::string ExternalLabelParameterName(const std::string& label_name,
                                          size_t i);
   std::string ExternalParameterName(const std::string& name);
 
-  std::ostream& source_out() { return CurrentNamespace()->source_stream(); }
-
-  std::ostream& header_out() { return CurrentNamespace()->header_stream(); }
-
+  std::ostream& source_out() {
+    Callable* callable = CurrentCallable::Get();
+    if (!callable || callable->ShouldGenerateExternalCode()) {
+      return CurrentNamespace()->source_stream();
+    } else {
+      return null_stream_;
+    }
+  }
+  std::ostream& header_out() {
+    Callable* callable = CurrentCallable::Get();
+    if (!callable || callable->ShouldGenerateExternalCode()) {
+      return CurrentNamespace()->header_stream();
+    } else {
+      return null_stream_;
+    }
+  }
   CfgAssembler& assembler() { return *assembler_; }
 
   void SetReturnValue(VisitResult return_value) {
@@ -503,6 +518,7 @@ class ImplementationVisitor : public FileVisitor {
   }
 
   base::Optional<CfgAssembler> assembler_;
+  NullOStream null_stream_;
 };
 
 }  // namespace torque

src/torque/utils.h

@@ -5,6 +5,8 @@
 #ifndef V8_TORQUE_UTILS_H_
 #define V8_TORQUE_UTILS_H_
 
+#include <ostream>
+#include <streambuf>
 #include <string>
 #include <unordered_set>
 #include <vector>
@@ -289,6 +291,25 @@ void EraseIf(Container* container, F f) {
   }
 }
 
+class NullStreambuf : public std::streambuf {
+ public:
+  virtual int overflow(int c) {
+    setp(buffer_, buffer_ + sizeof(buffer_));
+    return (c == traits_type::eof()) ? '\0' : c;
+  }
+
+ private:
+  char buffer_[64];
+};
+
+class NullOStream : public std::ostream {
+ public:
+  NullOStream() : std::ostream(&buffer_) {}
+
+ private:
+  NullStreambuf buffer_;
+};
+
 }  // namespace torque
 }  // namespace internal
 }  // namespace v8