Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable the expansion of header unions in the copy structures pass. #5093

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Disable the expansion of header unions in the copy structures pass. #5093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c64e941
Add an option to copy structures to skip expanding header unions.
fruffy Aug 7, 2024
1376d86
Review comments.
fruffy Jan 7, 2025
1a3989f
Reference files.
fruffy Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion backends/bmv2/pna_nic/midend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ PnaNicMidEnd::PnaNicMidEnd(CompilerOptions &options, std::ostream *outStream)
new P4::StrengthReduction(&typeMap),
new P4::EliminateTuples(&typeMap),
new P4::SimplifyComparisons(&typeMap),
new P4::CopyStructures(&typeMap),
new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig()),
new P4::NestedStructs(&typeMap),
new P4::SimplifySelectList(&typeMap),
new P4::RemoveSelectBooleans(&typeMap),
Expand Down
2 changes: 1 addition & 1 deletion backends/bmv2/psa_switch/midend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ PsaSwitchMidEnd::PsaSwitchMidEnd(CompilerOptions &options, std::ostream *outStre
new P4::StrengthReduction(&typeMap),
new P4::EliminateTuples(&typeMap),
new P4::SimplifyComparisons(&typeMap),
new P4::CopyStructures(&typeMap),
new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig()),
new P4::NestedStructs(&typeMap),
new P4::SimplifySelectList(&typeMap),
new P4::RemoveSelectBooleans(&typeMap),
Expand Down
2 changes: 1 addition & 1 deletion backends/bmv2/simple_switch/midend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ SimpleSwitchMidEnd::SimpleSwitchMidEnd(CompilerOptions &options, std::ostream *o
new P4::StrengthReduction(&typeMap),
new P4::EliminateTuples(&typeMap),
new P4::SimplifyComparisons(&typeMap),
new P4::CopyStructures(&typeMap),
new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig()),
new P4::NestedStructs(&typeMap),
new P4::SimplifySelectList(&typeMap),
new P4::RemoveSelectBooleans(&typeMap),
Expand Down
6 changes: 5 additions & 1 deletion backends/dpdk/midend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,11 @@ DpdkMidEnd::DpdkMidEnd(CompilerOptions &options, std::ostream *outStream) {
new P4::StrengthReduction(&typeMap),
new P4::EliminateTuples(&typeMap),
new P4::SimplifyComparisons(&typeMap),
new P4::CopyStructures(&typeMap, false /* errorOnMethodCall */),
new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig{
/*errorOnMethodCall*/ false,
/*copyHeaders*/ false,
/*expandUnions*/ true,
}),
new P4::NestedStructs(&typeMap),
new P4::SimplifySelectList(&typeMap),
new P4::RemoveSelectBooleans(&typeMap),
Expand Down
6 changes: 5 additions & 1 deletion backends/p4test/midend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,11 @@ MidEnd::MidEnd(CompilerOptions &options, std::ostream *outStream) {
new P4::EliminateTuples(&typeMap),
new P4::FlattenLogMsg(&typeMap),
new P4::SimplifyComparisons(&typeMap),
new P4::CopyStructures(&typeMap, false),
new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig{
/*errorOnMethodCall*/ false,
/*copyHeaders*/ false,
/*expandUnions*/ true,
}),
new P4::NestedStructs(&typeMap),
new P4::StrengthReduction(&typeMap),
new P4::SimplifySelectList(&typeMap),
Expand Down
8 changes: 7 additions & 1 deletion backends/p4tools/common/compiler/midend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,13 @@ void MidEnd::addDefaultPasses() {
new P4::SimplifyComparisons(&typeMap),
// Expand header and struct assignments into sequences of field assignments.
new PassRepeated({
new P4::CopyStructures(&typeMap, false, true, nullptr),
new P4::CopyStructures(&typeMap,
{
/*errorOnMethodCall*/ false,
/*copyHeaders*/ true,
/*expandUnions*/ false,
},
nullptr),
}),
new P4::RemoveParserControlFlow(&typeMap),
// Flatten nested list expressions.
Expand Down
2 changes: 1 addition & 1 deletion backends/tofino/bf-p4c/arch/psa/psa.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
8 changes: 6 additions & 2 deletions backends/tofino/bf-p4c/midend/copy_header.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
2 changes: 1 addition & 1 deletion backends/ubpf/midend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ const IR::ToplevelBlock *MidEnd::run(EbpfOptions &options, const IR::P4Program *
new P4::StrengthReduction(&typeMap),
}),
new P4::SimplifyComparisons(&typeMap),
new P4::CopyStructures(&typeMap),
new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig()),
new P4::LocalCopyPropagation(&typeMap),
new P4::SimplifySelectList(&typeMap),
new P4::MoveDeclarations(), // more may have been introduced
Expand Down
2 changes: 1 addition & 1 deletion frontends/p4/toP4/toP4.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ bool ToP4::preorder(const IR::Type_Error *d) {
})
->toVector();
if (!isDeclaration || !userErrors.empty()) {
builder.append("error");
builder.append("error ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unrelated. Also, it would be better to print the space only after the if (!isDeclaration).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional. Fixes a bug introduced by #5036. If you do not change this line you will produce incorrect reference files. I am actually a little concerned why this is not picked up in checks.


if (!isDeclaration) {
return false;
Expand Down
9 changes: 5 additions & 4 deletions midend/copyStructures.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,14 @@ const IR::Node *DoCopyStructures::postorder(IR::AssignmentStatement *statement)
FIXME: this is not correct for header unions and should be fixed.
The fix bellow, commented-out, causes problems elsewhere.
https://github.com/p4lang/p4c/issues/3842
if (ltype->is<IR::Type_HeaderUnion>())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, but it appears that these changes are attempting to fix the issue #3842 mentioned in comments just before this line, and thus those comments can be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the current fix just disables union expansion by default to avoid running into #3842. Ideally, we fix the problem in the copyStructures pass somehow by tracking validity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this PR is not fixing that issue, if I understand the changes correctly. You are just disabling incorrect transformations.

return statement;
*/
if (!config.expandUnions && ltype->is<IR::Type_HeaderUnion>()) {
return statement;
}

// Do not copy structures for method calls.
if (statement->right->is<IR::MethodCallExpression>()) {
if (errorOnMethodCall) {
if (config.errorOnMethodCall) {
::P4::error(ErrorType::ERR_UNSUPPORTED_ON_TARGET,
"%1%: functions or methods returning structures "
"are not supported on this target",
Expand All @@ -98,7 +99,7 @@ const IR::Node *DoCopyStructures::postorder(IR::AssignmentStatement *statement)
retval.push_back(
new IR::AssignmentStatement(statement->srcInfo, left, right->expression));
}
} else if (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(
Expand Down
37 changes: 25 additions & 12 deletions midend/copyStructures.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,23 @@ limitations under the License.

namespace P4 {

struct CopyStructuresConfig {
/// Specific targets may allow functions or methods to return structs.
/// Such methods will not be converted in this pass. Setting the
/// errorOnMethodCall flag will produce an error message if such a
/// method is encountered.
bool errorOnMethodCall = true;

/// Do not only copy normal structures but also perform copy assignments for headers.
bool copyHeaders = false;
/// Also expand header union assignments.
/// 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.
/// Some passes, such as FlattenHeaderUnion requires this expansion.
bool expandUnions = false;
};

/**
* Convert assignments between structures to assignments between fields
*
Expand Down Expand Up @@ -53,19 +70,15 @@ namespace P4 {
*
*/
class DoCopyStructures : public Transform {
/// The type map.
TypeMap *typeMap;
/// Specific targets may allow functions or methods to return structs.
/// Such methods will not be converted in this pass. Setting the
/// errorOnMethodCall flag will produce an error message if such a
/// method is encountered.
bool errorOnMethodCall;

/// Do not only copy normal structures but also perform copy assignments for headers.
bool copyHeaders;
/// Configuration options.
CopyStructuresConfig config;

public:
explicit DoCopyStructures(TypeMap *typeMap, bool errorOnMethodCall, bool copyHeaders = false)
: typeMap(typeMap), errorOnMethodCall(errorOnMethodCall), copyHeaders(copyHeaders) {
explicit DoCopyStructures(TypeMap *typeMap, CopyStructuresConfig configuration)
: typeMap(typeMap), config(configuration) {
CHECK_NULL(typeMap);
setName("DoCopyStructures");
}
Expand Down Expand Up @@ -116,15 +129,15 @@ class RemoveAliases : public Transform {

class CopyStructures : public PassRepeated {
public:
explicit CopyStructures(TypeMap *typeMap, bool errorOnMethodCall = true,
bool copyHeaders = false, TypeChecking *typeChecking = nullptr) {
explicit CopyStructures(TypeMap *typeMap, CopyStructuresConfig config,
TypeChecking *typeChecking = nullptr) {
CHECK_NULL(typeMap);
setName("CopyStructures");
if (typeChecking == nullptr) typeChecking = new TypeChecking(nullptr, typeMap);
passes.emplace_back(typeChecking);
passes.emplace_back(new RemoveAliases(typeMap));
passes.emplace_back(typeChecking);
passes.emplace_back(new DoCopyStructures(typeMap, errorOnMethodCall, copyHeaders));
passes.emplace_back(new DoCopyStructures(typeMap, config));
}
};

Expand Down
2 changes: 1 addition & 1 deletion test/gtest/midend_pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ MidEnd::MidEnd(CompilerOptions &options, std::ostream *outStream) {
new P4::StrengthReduction(&typeMap),
new P4::EliminateTuples(&typeMap),
new P4::SimplifyComparisons(&typeMap),
new P4::CopyStructures(&typeMap),
new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig()),
new P4::NestedStructs(&typeMap),
new P4::StrengthReduction(&typeMap),
new P4::SimplifySelectList(&typeMap),
Expand Down
33 changes: 9 additions & 24 deletions testdata/p4_16_samples_outputs/invalid-hdr-warnings5-midend.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ header Header2 {
bit<16> data;
}

header_union Union {
Header1 h1;
Header2 h2;
Header1 h3;
}

struct H {
Header1 h1;
Header1 u1_h1;
Expand Down Expand Up @@ -77,9 +83,11 @@ control IngressI(inout H hdr, inout M meta, inout standard_metadata_t smeta) {
@name("u1_0_h1") Header1 u1_0_h1_0;
@name("u1_0_h2") Header2 u1_0_h2_0;
@name("u1_0_h3") Header1 u1_0_h3_0;
@name("IngressI.u1") Union u1;
@name("u2_0_h1") Header1 u2_0_h1_0;
@name("u2_0_h2") Header2 u2_0_h2_0;
@name("u2_0_h3") Header1 u2_0_h3_0;
@name("IngressI.u2") Union u2;
@hidden @name("invalidhdrwarnings5l58") action invalidhdrwarnings5l58_0() {
u1_0_h1_0.setInvalid();
u1_0_h2_0.setInvalid();
Expand All @@ -97,30 +105,7 @@ control IngressI(inout H hdr, inout M meta, inout standard_metadata_t smeta) {
u1_0_h2_0.setValid();
u1_0_h1_0.setInvalid();
u1_0_h3_0.setInvalid();
if (u1_0_h1_0.isValid()) {
u2_0_h1_0.setValid();
u2_0_h1_0 = u1_0_h1_0;
u2_0_h2_0.setInvalid();
u2_0_h3_0.setInvalid();
} else {
u2_0_h1_0.setInvalid();
}
if (u1_0_h2_0.isValid()) {
u2_0_h2_0.setValid();
u2_0_h2_0 = u1_0_h2_0;
u2_0_h1_0.setInvalid();
u2_0_h3_0.setInvalid();
} else {
u2_0_h2_0.setInvalid();
}
if (u1_0_h3_0.isValid()) {
u2_0_h3_0.setValid();
u2_0_h3_0 = u1_0_h3_0;
u2_0_h1_0.setInvalid();
u2_0_h2_0.setInvalid();
} else {
u2_0_h3_0.setInvalid();
}
u2 = u1;
u2_0_h2_0.setValid();
u2_0_h2_0.data = 16w1;
u2_0_h1_0.setInvalid();
Expand Down
25 changes: 1 addition & 24 deletions testdata/p4_16_samples_outputs/invalid-hdr-warnings6-midend.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,30 +105,7 @@ control IngressI(inout H hdr, inout M meta, inout standard_metadata_t smeta) {
u_1[0].h2.setValid();
u_1[0].h1.setInvalid();
u_1[0].h3.setInvalid();
if (u_1[1w0].h1.isValid()) {
u_1[1].h1.setValid();
u_1[1].h1 = u_1[1w0].h1;
u_1[1].h2.setInvalid();
u_1[1].h3.setInvalid();
} else {
u_1[1].h1.setInvalid();
}
if (u_1[1w0].h2.isValid()) {
u_1[1].h2.setValid();
u_1[1].h2 = u_1[1w0].h2;
u_1[1].h1.setInvalid();
u_1[1].h3.setInvalid();
} else {
u_1[1].h2.setInvalid();
}
if (u_1[1w0].h3.isValid()) {
u_1[1].h3.setValid();
u_1[1].h3 = u_1[1w0].h3;
u_1[1].h1.setInvalid();
u_1[1].h2.setInvalid();
} else {
u_1[1].h3.setInvalid();
}
u_1[1] = u_1[1w0];
u_1[1].h2.setValid();
u_1[1].h2.data = 16w1;
u_1[1].h1.setInvalid();
Expand Down
Loading
Loading