Skip to content

Move some P4Tool compiler passes to the midend folder. #3726

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

Jump to bottom
Merged
merged 7 commits into from
Nov 28, 2022
Merged

Move some P4Tool compiler passes to the midend folder. #3726

merged 7 commits into from
Nov 28, 2022

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Nov 23, 2022 •
edited
Loading

Fixes #3518.

I merged the CopyHeaders pass into the CopyStructures pass. This should be checked since I am not sure whether I preserved its semantics correctly.

Also there is a question whether coverage.cpp should become a part of the midend or remain part of tools. I think it can be generally useful.

@fruffy fruffy force-pushed the fruffy/compiler_passes branch 2 times, most recently from 27c9148 to 4c41ce4 Compare November 25, 2022 16:41
@fruffy fruffy marked this pull request as ready for review November 25, 2022 17:49
@mihaibudiu
Copy link
Contributor

These passes should probably have been moved in separate PRs.
Reviewing a very large PR is much harder than reviewer many small ones.
This is especially true if there are changes requested to the PR.

@mihaibudiu
Copy link
Contributor

I think you should consider breaking this PR into pieces, shouldn't be too difficult I expect.

@fruffy
Copy link
Collaborator Author

fruffy commented Nov 25, 2022

I can split off the CopyStructures pass. The other passes require few file changes.

@fruffy fruffy force-pushed the fruffy/compiler_passes branch from 4c41ce4 to 66993ee Compare November 25, 2022 18:08
fruffy
fruffy commented Nov 25, 2022
View reviewed changes
midend/coverage.h
@@ -8,7 +8,12 @@
#include "ir/visitor.h"
#include "lib/source_file.h"

namespace P4Tools {
/// This file is a collection of utilities for coverage tracking in P4 programs. Currently, this
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The question is whether this file should stile in tools or move to the mid end. If there is no use for it, it can stay in tools.

midend/convertErrors.h
@@ -16,98 +16,77 @@ limitations under the License.
#ifndef _MIDEND_CONVERTERRORS_H_
#define _MIDEND_CONVERTERRORS_H_

#include <map>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The toosl version of convertErrors was merged with the mid end version. They are basically equivalent, it is just that the mid end version did not have a cpp file.

midend/hsIndexSimplify.cpp
@@ -29,9 +29,9 @@ void HSIndexFinder::addNewVariable() {
newVariable = arrayIndex->right->to<IR::PathExpression>();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All the edits here are just linter fixes.

midend/hsIndexSimplify.cpp Outdated
@@ -225,4 +224,17 @@ IR::Node* HSIndexContretizer::preorder(IR::SwitchStatement* switchStatement) {
return eliminateArrayIndexes(aiFinder, switchStatement, nullptr);
}

const IR::Node* HSIndexToMember::postorder(IR::ArrayIndex* curArrayIndex) {
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 class was merged into the hsIndexSimplify file.

midend/hsIndexSimplify.h Outdated
@@ -102,6 +102,19 @@ class HSIndexSimplifier : public PassManager {
}
};

/// Replaces all ArrayIndex classes with a member classes that have the integer index as string
/// member.
/// WARNING: After applying this pass, the IR will not type check any longer.
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 important. Maybe worth printing a warning when invoking this pass? But this could be considered spam.

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not really make it a midend pass, these are supposed to be mixed and matched, but if you run this you're finished.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can move this back then. It is a utility pass in some form.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you move it back?

@fruffy fruffy force-pushed the fruffy/compiler_passes branch from 66993ee to d96486d Compare November 25, 2022 18:29
@fruffy
Copy link
Collaborator Author

fruffy commented Nov 27, 2022

I split the mid end passes into individual commits.

mihaibudiu
mihaibudiu approved these changes Nov 28, 2022
View reviewed changes
midend/booleanKeys.h Outdated
*
* \code{.cpp}
* table t {
* key = { (bit<1<) h.a : exact; }
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

midend/booleanKeys.h
* \endcode
*
* @pre none
* @post all boolean table key expressions are replaced with a bit<1> cast. Constant entries are
Copy link
Contributor

Choose a reason for hiding this comment

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

any problems with the control-plane API after this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will check with a dummy commit. There might be issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

p4c/backends/bmv2/common/expression.cpp

Line 621 in e26a49d

::error(ErrorType::ERR_UNSUPPORTED_ON_TARGET, "%1%: expression too complex for this target",

This error will fire if we use the pass for the simple switch.

midend/convertErrors.h

namespace P4 {

/**
* Policy function: Determines if the error type should be converted to Ser_Enum.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you removing this code?
Are you sure no one needs it?

midend/copyStructures.cpp
auto tmp = refMap->newName("tmp");
auto decl = new IR::Declaration_Variable(IR::ID(tmp), type->getP4Type(), nullptr);
auto* decl = new IR::Declaration_Variable(IR::ID(tmp), type->getP4Type(), nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really think these improve the code in any way.

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 a clang-tidy recommendation to comply with this https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto. I do not think it can hurt to use references by default.

midend/hsIndexSimplify.h Outdated
@@ -102,6 +102,19 @@ class HSIndexSimplifier : public PassManager {
}
};

/// Replaces all ArrayIndex classes with a member classes that have the integer index as string
/// member.
/// WARNING: After applying this pass, the IR will not type check any longer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you move it back?

@fruffy fruffy force-pushed the fruffy/compiler_passes branch 2 times, most recently from b652d7d to 28abe02 Compare November 28, 2022 20:44
@fruffy
Copy link
Collaborator Author

fruffy commented Nov 28, 2022

@mbudiu-vmw Are all discussion points resolved?

@mihaibudiu
Copy link
Contributor

I thought you were planning to move hsIndex back into tools.

@fruffy
Copy link
Collaborator Author

fruffy commented Nov 28, 2022

I did. The other code was there before.

@fruffy fruffy merged commit d7c635e into main Nov 28, 2022
@fruffy fruffy mentioned this pull request Nov 29, 2022
Merged
@fruffy fruffy deleted the fruffy/compiler_passes branch November 30, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihaibudiu mihaibudiu mihaibudiu approved these changes

@pkotikal pkotikal Awaiting requested review from pkotikal

@vhavel vhavel Awaiting requested review from vhavel

@jnfoster jnfoster Awaiting requested review from jnfoster

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Contribute compiler passes from testgen to midend folder
2 participants
@fruffy @mihaibudiu