Fix issue 236: Repeated --import_dirs in cpp_golden_test

compiler/back_end/cpp/BUILD

@@ -388,6 +388,17 @@ emboss_cc_test(
     ],
 )
 
+emboss_cc_test(
+    name = "many_conditionals_benchmark",
+    srcs = [
+        "testcode/many_conditionals_benchmark.cc",
+    ],
+    deps = [
+        "//testdata:many_conditionals_emboss",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 # New golden test infrastructure
 py_library(
     name = "one_golden_test_lib",
@@ -535,6 +546,7 @@ cpp_golden_test(
 cpp_golden_test(
     name = "no_enum_traits_golden_test",
     emb_file = "//testdata:no_enum_traits.emb",
+    extra_args = ["--no-cc-enum-traits"],
     golden_file = "//testdata/golden_cpp:no_enum_traits.emb.h",
 )
 

compiler/back_end/cpp/build_defs.bzl

@@ -46,15 +46,21 @@ def emboss_cc_test(name, copts = None, no_w_sign_compare = False, **kwargs):
         **kwargs
     )
 
-def cpp_golden_test(name, emb_file, golden_file, import_dirs = []):
+def cpp_golden_test(name, emb_file, golden_file, import_dirs = [], extra_args = []):
     """Defines a C++ golden file test.
 
     Args:
         name: The name of the test.
         emb_file: The .emb file to test.
         golden_file: The golden .h file.
         import_dirs: A list of import directories.
+        extra_args: A list of extra arguments to pass to the compiler.
     """
+    unique_import_dirs = []
+    for d in import_dirs:
+        if d not in unique_import_dirs:
+            unique_import_dirs.append(d)
+
     py_test(
         name = name,
         main = ":run_one_golden_test.py",
@@ -65,13 +71,13 @@ def cpp_golden_test(name, emb_file, golden_file, import_dirs = []):
             "$(location :emboss_codegen_cpp)",
             "$(location %s)" % emb_file,
             "$(location %s)" % golden_file,
-        ] + ["--import-dir=" + d for d in import_dirs],
+        ] + ["--import-dir=" + d for d in unique_import_dirs] + extra_args,
         data = [
             "//compiler/front_end:emboss_front_end",
             ":emboss_codegen_cpp",
             emb_file,
             golden_file,
             "//testdata:test_embs",
-        ] + import_dirs,
+        ] + unique_import_dirs,
         deps = [":one_golden_test_lib"],
     )

compiler/back_end/cpp/generated_code_templates

@@ -127,6 +127,7 @@ class Generic${name}View final {
   bool Ok() const {
     if (!IsComplete()) return false;
 ${parameter_ok_checks}
+${ok_subexpressions}
 ${field_ok_checks}
 ${requires_check}
     return true;
@@ -391,11 +392,14 @@ ${write_fields}
 // ** ok_method_test ** ////////////////////////////////////////////////////////
     // If we don't have enough information to determine whether ${field} is
     // present in the structure, then structure.Ok() should be false.
-    if (!has_${field}.Known()) return false;
-    // If ${field} is present, but not Ok(), then structure.Ok() should be
-    // false.  If ${field} is not present, it does not matter whether it is
-    // Ok().
-    if (has_${field}.ValueOrDefault() && !${field}.Ok()) return false;
+    {
+      const auto emboss_reserved_local_field_present = ${existence_condition};
+      if (!emboss_reserved_local_field_present.Known()) return false;
+      // If ${field} is present, but not Ok(), then structure.Ok() should be
+      // false.  If ${field} is not present, it does not matter whether it is
+      // Ok().
+      if (emboss_reserved_local_field_present.ValueOrDefault() && !${field}.Ok()) return false;
+    }
 
 
 // ** equals_method_test ** ////////////////////////////////////////////////////

compiler/back_end/cpp/header_generator.py

@@ -791,6 +791,44 @@ def subexprs(self):
         ]
 
 
+class _UsageCountingStore(object):
+    """A mock SubexpressionStore that counts subexpression usage."""
+
+    def __init__(self):
+        self.counts = collections.defaultdict(int)
+
+    def add(self, subexpr):
+        self.counts[subexpr] += 1
+        return subexpr
+
+
+class _SmartSubexpressionStore(object):
+    """A SubexpressionStore that only caches subexpressions used multiple times."""
+
+    def __init__(self, prefix, counts):
+        self._prefix = prefix
+        self._counts = counts
+        self._subexpr_to_name = {}
+        self._index_to_subexpr = []
+
+    def add(self, subexpr):
+        if self._counts[subexpr] <= 1:
+            return subexpr
+
+        if subexpr not in self._subexpr_to_name:
+            self._index_to_subexpr.append(subexpr)
+            self._subexpr_to_name[subexpr] = self._prefix + str(
+                len(self._index_to_subexpr)
+            )
+        return self._subexpr_to_name[subexpr]
+
+    def subexprs(self):
+        return [
+            (self._subexpr_to_name[subexpr], subexpr)
+            for subexpr in self._index_to_subexpr
+        ]
+
+
 _ExpressionResult = collections.namedtuple(
     "ExpressionResult", ["rendered", "is_constant"]
 )
@@ -871,7 +909,9 @@ def _render_expression(expression, ir, field_reader=None, subexpressions=None):
 
 
 def _render_existence_test(field, ir, subexpressions=None):
-    return _render_expression(field.existence_condition, ir, subexpressions)
+    return _render_expression(
+        field.existence_condition, ir, subexpressions=subexpressions
+    )
 
 
 def _alignment_of_location(location):
@@ -1421,6 +1461,16 @@ def _generate_structure_definition(type_ir, ir, config: Config):
         initialize_parameters_initialized_true = ""
         parameter_checks = [""]
 
+    # Pass 1: Count subexpression usage.
+    ok_usage_counter = _UsageCountingStore()
+    for field_index in type_ir.structure.fields_in_dependency_order:
+        field = type_ir.structure.field[field_index]
+        _render_existence_test(field, ir, ok_usage_counter)
+
+    # Pass 2: Generate code using smart store.
+    ok_subexpressions = _SmartSubexpressionStore(
+        "emboss_reserved_local_ok_subexpr_", ok_usage_counter.counts
+    )
     for field_index in type_ir.structure.fields_in_dependency_order:
         field = type_ir.structure.field[field_index]
         helper_types, declaration, definition = _generate_structure_field_methods(
@@ -1432,6 +1482,9 @@ def _generate_structure_definition(type_ir, ir, config: Config):
             code_template.format_template(
                 _TEMPLATES.ok_method_test,
                 field=_cpp_field_name(field.name.name.text) + "()",
+                existence_condition=_render_existence_test(
+                    field, ir, subexpressions=ok_subexpressions
+                ).rendered,
             )
         )
         if not ir_util.field_is_virtual(field):
@@ -1499,6 +1552,12 @@ def _generate_structure_definition(type_ir, ir, config: Config):
         size_method=_render_size_method(type_ir.structure.field, ir),
         field_method_declarations="".join(field_method_declarations),
         field_ok_checks="\n".join(ok_method_clauses),
+        ok_subexpressions="".join(
+            [
+                "    const auto {} = {};\n".format(name, subexpr)
+                for name, subexpr in ok_subexpressions.subexprs()
+            ]
+        ),
         parameter_ok_checks="\n".join(parameter_checks),
         requires_check=requires_check,
         equals_method_body="\n".join(equals_method_clauses),

compiler/back_end/cpp/one_golden_test.py

@@ -27,13 +27,15 @@ def __init__(
         emb_file,
         golden_file,
         include_dirs=None,
+        compiler_flags=None,
     ):
         super(OneGoldenTest, self).__init__("test_golden_file")
         self.emboss_front_end = emboss_front_end
         self.emboss_compiler = emboss_compiler
         self.emb_file = emb_file
         self.golden_file = golden_file
         self.include_dirs = include_dirs if include_dirs is not None else []
+        self.compiler_flags = compiler_flags if compiler_flags is not None else []
 
     def test_golden_file(self):
         temp_dir = os.environ.get("TEST_TMPDIR", "")
@@ -61,6 +63,7 @@ def test_golden_file(self):
             "--output-file",
             output_path,
         ]
+        compiler_args.extend(self.compiler_flags)
 
         process = subprocess.run(compiler_args, capture_output=True, text=True)
 

compiler/back_end/cpp/run_one_golden_test.py

@@ -34,12 +34,24 @@ def main(argv):
     emboss_compiler = argv[2]
     emb_file = argv[3]
     golden_file = argv[4]
-    include_dirs = argv[5:]
+
+    include_dirs = []
+    compiler_flags = []
+    for arg in argv[5:]:
+        if arg.startswith("--import-dir="):
+            include_dirs.append(arg[len("--import-dir=") :])
+        else:
+            compiler_flags.append(arg)
 
     suite = unittest.TestSuite()
     suite.addTest(
         OneGoldenTest(
-            emboss_front_end, emboss_compiler, emb_file, golden_file, include_dirs
+            emboss_front_end,
+            emboss_compiler,
+            emb_file,
+            golden_file,
+            include_dirs,
+            compiler_flags,
         )
     )
     runner = unittest.TextTestRunner()

compiler/back_end/cpp/testcode/many_conditionals_benchmark.cc

@@ -0,0 +1,45 @@
+#include <gtest/gtest.h>
+
+#include <chrono>
+#include <iostream>
+#include <vector>
+
+#include "testdata/many_conditionals.emb.h"
+
+// A simple test that acts as a benchmark/sanity check.
+// Since this file is in compiler/back_end/cpp/testcode/, it will be built as a
+// cc_test. We can use GoogleTest macros.
+
+namespace emboss {
+namespace test {
+namespace {
+
+TEST(ComplexConditionals, PerformanceBenchmark) {
+  std::vector<char> buffer(100, 0);
+  auto view = emboss::test::MakeLargeConditionalsView(&buffer);
+
+  auto start = std::chrono::high_resolution_clock::now();
+  int iterations = 10000;
+  volatile bool result = false;
+  for (int i = 0; i < iterations; ++i) {
+    for (int tag = 0; tag < 100; ++tag) {
+      view.tag().Write(tag);
+      result = view.Ok();
+    }
+  }
+  auto end = std::chrono::high_resolution_clock::now();
+
+  std::chrono::duration<double> elapsed = end - start;
+  // We don't strictly fail on time, but we print it.
+  // In a real CI system we might assert upper bounds.
+  std::cout << "Time for " << iterations
+            << " iterations (x100 tags): " << elapsed.count() << "s"
+            << std::endl;
+
+  EXPECT_TRUE(result);
+  EXPECT_TRUE(view.Ok());
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace emboss

compiler/util/parser_types.py

@@ -15,7 +15,7 @@
 """Types related to the LR(1) parser.
 
 This module contains types used by the LR(1) parser, which are also used in
-other parts of the compiler: 
+other parts of the compiler:
 
     SourcePosition: a position (zero-width) within a source file.
     SourceLocation: a span within a source file.

testdata/BUILD

@@ -36,11 +36,8 @@ filegroup(
     ],
 )
 
-
-
 exports_files(glob(["**/*.emb"]))
 
-
 filegroup(
     name = "test_embs",
     srcs = [
@@ -369,3 +366,10 @@ emboss_cc_library(
         "complex_offset.emb",
     ],
 )
+
+emboss_cc_library(
+    name = "many_conditionals_emboss",
+    srcs = [
+        "many_conditionals.emb",
+    ],
+)