Review comments.

Signed-off-by: fruffy <[email protected]>
p4lang · Jan 7, 2025 · 4dac5d4 · 4dac5d4
1 parent c64e941
commit 4dac5d4
Show file tree

Hide file tree

Showing 7 changed files with 20 additions and 15 deletions.
diff --git a/backends/dpdk/midend.cpp b/backends/dpdk/midend.cpp
@@ -197,7 +197,7 @@ DpdkMidEnd::DpdkMidEnd(CompilerOptions &options, std::ostream *outStream) {
             new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig{
                                                  /*errorOnMethodCall*/ false,
                                                  /*copyHeaders*/ false,
-                                                 /*expandUnions*/ true,
+                                                 /*expandUnions*/ false,
                                              }),
             new P4::NestedStructs(&typeMap),
             new P4::SimplifySelectList(&typeMap),

diff --git a/backends/p4test/midend.cpp b/backends/p4test/midend.cpp
@@ -107,7 +107,7 @@ MidEnd::MidEnd(CompilerOptions &options, std::ostream *outStream) {
          new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig{
                                               /*errorOnMethodCall*/ false,
                                               /*copyHeaders*/ false,
-                                              /*expandUnions*/ true,
+                                              /*expandUnions*/ false,
                                           }),
          new P4::NestedStructs(&typeMap),
          new P4::StrengthReduction(&typeMap),

diff --git a/backends/tofino/bf-p4c/arch/psa/psa.cpp b/backends/tofino/bf-p4c/arch/psa/psa.cpp
@@ -1070,7 +1070,7 @@ PortableSwitchTranslation::PortableSwitchTranslation(P4::ReferenceMap *refMap, P
                                          "psa_direct_meter"_cs, "psa_idle_timeout"_cs,
                                          "psa_empty_group_action"_cs}),
         new P4::ConvertEnums(typeMap, new PSA::PacketPathTo8Bits),
-        new P4::CopyStructures(typeMap),
+        new P4::CopyStructures(typeMap, P4::CopyStructuresConfig()),
         new BFN::TypeChecking(refMap, typeMap, true),
         evaluator,
         new VisitFunctor(

diff --git a/backends/tofino/bf-p4c/midend/copy_header.cpp b/backends/tofino/bf-p4c/midend/copy_header.cpp
@@ -265,7 +265,11 @@ BFN::CopyHeaders::CopyHeaders(P4::ReferenceMap *refMap, P4::TypeMap *typeMap,
     passes.emplace_back(typeChecking);
     // `errorOnMethodCall = false` viz don't flag functions returning structs as an error.
     // E.g. Phase0 extern function returns a header struct.
-    passes.emplace_back(new P4::DoCopyStructures(typeMap, false));
-    passes.emplace_back(typeChecking);
+    new P4::CopyStructures(&typeMap, P4::DoCopyStructures{
+                                         /*errorOnMethodCall*/ false,
+                                         /*copyHeaders*/ false,
+                                         /*expandUnions*/ false,
+                                     }),
+        passes.emplace_back(typeChecking);
     passes.emplace_back(new DoCopyHeaders(typeMap));
 }
diff --git a/frontends/p4/toP4/toP4.cpp b/frontends/p4/toP4/toP4.cpp
@@ -660,7 +660,7 @@ bool ToP4::preorder(const IR::Type_Error *d) {
                                 })
                                 ->toVector();
     if (!isDeclaration || !userErrors.empty()) {
-        builder.append("error");
+        builder.append("error ");
 
         if (!isDeclaration) {
             return false;

diff --git a/midend/copyStructures.cpp b/midend/copyStructures.cpp
@@ -74,13 +74,13 @@ const IR::Node *DoCopyStructures::postorder(IR::AssignmentStatement *statement)
        The fix bellow, commented-out, causes problems elsewhere.
        https://github.com/p4lang/p4c/issues/3842
     */
-    if (!_config.expandUnions && ltype->is<IR::Type_HeaderUnion>()) {
+    if (!config.expandUnions && ltype->is<IR::Type_HeaderUnion>()) {
         return statement;
     }
 
     // Do not copy structures for method calls.
     if (statement->right->is<IR::MethodCallExpression>()) {
-        if (_config.errorOnMethodCall) {
+        if (config.errorOnMethodCall) {
             ::P4::error(ErrorType::ERR_UNSUPPORTED_ON_TARGET,
                         "%1%: functions or methods returning structures "
                         "are not supported on this target",
@@ -99,7 +99,7 @@ const IR::Node *DoCopyStructures::postorder(IR::AssignmentStatement *statement)
             retval.push_back(
                 new IR::AssignmentStatement(statement->srcInfo, left, right->expression));
         }
-    } else if (_config.copyHeaders && ltype->is<IR::Type_Header>()) {
+    } else if (config.copyHeaders && ltype->is<IR::Type_Header>()) {
         const auto *header = ltype->checkedTo<IR::Type_Header>();
         // Build a "src.isValid()" call.
         const auto *isSrcValidCall = new IR::MethodCallExpression(

diff --git a/midend/copyStructures.h b/midend/copyStructures.h
@@ -32,9 +32,10 @@ struct CopyStructuresConfig {
     /// Do not only copy normal structures but also perform copy assignments for headers.
     bool copyHeaders = false;
     /// Also expand header union assignments.
-    /// TODO: This is only necessary because the copy structure pass does not correctly expand
-    /// header unions for some back ends.
-    bool expandUnions = true;
+    /// TODO: This is currently disabled by default because validity of header unions is not
+    /// correctly preserved. Only the validity of the last member in the union is preserved in the
+    /// assignment..
+    bool expandUnions = false;
 };
 
 /**
@@ -72,11 +73,11 @@ class DoCopyStructures : public Transform {
     TypeMap *typeMap;
 
     /// Configuration options.
-    CopyStructuresConfig _config;
+    CopyStructuresConfig config;
 
  public:
-    explicit DoCopyStructures(TypeMap *typeMap, CopyStructuresConfig config)
-        : typeMap(typeMap), _config(config) {
+    explicit DoCopyStructures(TypeMap *typeMap, CopyStructuresConfig configuration)
+        : typeMap(typeMap), config(configuration) {
         CHECK_NULL(typeMap);
         setName("DoCopyStructures");
     }