Implement parser inlining #241

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

Closed

mtsamis wants to merge 4 commits into p4lang:main from mtsamis:mtsamis/inline-subparsers-new

Collaborator

mtsamis commented Oct 24, 2025

This PR implements parser inlining (#100).

The changes include generic infrastructure found in IRUtils and an inlining pass based on MLIR InliningUtils and the new SplitStateRewriter to handle parser rewrites. The implementation makes sure to leave the IR in its original state if the inlining fails at any part (e.g. due to an apply call being inside an if statement).

Although not necessary for correctness, parser locals are shared per instantiation to avoid creating too many new locals that will be hard to eliminate later on.

A number of tests are added, including the parser inlining tests from P4C. The latter are there only to check against potential crashes.

mtsamis requested a review from asl

October 24, 2025 14:02

Collaborator

asl commented Oct 24, 2025

Tagging @fruffy for visibility

fruffy approved these changes

View reviewed changes

Collaborator

fruffy left a comment •

edited

Loading

The lack of E2E testing and reference files makes it quite hard to tell whether the resulting semantic behavior is correct. Would be really nice to have a packet testing framework for this at some point.

Collaborator

asl commented Oct 26, 2025

@fruffy I'm afraid this will only be possible if / when we will have some backend implementation...

asl reviewed

View reviewed changes

Collaborator

asl left a comment

Made first sweep, check the comments below and above

include/p4mlir/Transforms/IRUtils.h Show resolved Hide resolved

include/p4mlir/Transforms/IRUtils.h Show resolved Hide resolved

lib/Transforms/IRUtils.cpp Outdated

+                                   llvm::StringRef varName = "promoted_local") {
+                  // Create new variable to hold `val`.
+                  auto newVar = rewriter.create<P4HIR::VariableOp>(
+                      val.getLoc(), P4HIR::ReferenceType::get(val.getType()), varName);

Collaborator

asl Oct 26, 2025

Add init attribute as the variable will be immediately initialized.

lib/Transforms/IRUtils.cpp Outdated

+              P4HIR::ParserStateOp IRUtils::createSubState(mlir::RewriterBase &rewriter,
+                                                           P4HIR::ParserStateOp state, llvm::StringRef suffix) {
+                  auto parser = state->getParentOfType<P4HIR::ParserOp>();
+                  std::string basename = (llvm::Twine(state.getSymName()) + "_" + suffix).str();

Collaborator

asl Oct 26, 2025

Do you really need Twine here? I'd suggest using SmallString here with pre-allocated buffer

lib/Transforms/IRUtils.cpp Outdated

+                  std::string name = basename;
+                  size_t counter = 0;
+                  while (parser.lookupSymbol(name)) name = basename + "_" + std::to_string(counter++);

Collaborator

asl Oct 26, 2025

Use mlir::SymbolTable::generateSymbolName. There are examples of usage in P4C/translate.cpp

Collaborator Author

mtsamis Oct 27, 2025

This made me realize that I merged an outdated version of IRUtils in this new branch. I will update it to the correct one.

lib/Transforms/InlineParsers.cpp Outdated

+              void InlineParsersPass::runOnOperation() {
+                  mlir::IRRewriter rewriter(&getContext());
+                  // P4 guarantees that subparser definitions come before their use so inlining them

Collaborator

asl Oct 26, 2025

This is true for P4 input source. But technically since we're referring things via symbols and symbol tables this might be not true anymore and this is not enforced in P4HIR. I would explicitly have a worklist and iterate until it is not empty

Collaborator Author

mtsamis Oct 27, 2025

Good point, I will re-evaluate the assumption and change the implementation as needed.

lib/Transforms/InlineParsers.cpp

+                          status = ssr->init();
+                          if (status.failed()) break;
+                          std::string prefix = instName.str();

Collaborator

asl Oct 26, 2025

use SmallString here

lib/Transforms/InlineParsers.cpp Outdated

+                      // Move inlined operation `op` to its desired position.
+                      void proccessInlinedOp(size_t index, mlir::Operation *op) {
+                          // Remmap parser locals if needed.

Collaborator

asl Oct 26, 2025

Suggested change

      
                        // Remmap parser locals if needed.
          
                        // Remap parser locals if needed.

lib/Transforms/InlineParsers.cpp

+                              // Make the subparser's accept transition to the "post" state.
+                              if (stateOp.isAccept()) {
+                                  auto acceptOp = mlir::cast<P4HIR::ParserAcceptOp>(stateOp.getNextTransition());

Collaborator

asl Oct 26, 2025

Should we just userewriter.replaceOpWithNewOp<P4HIR::ParserTransitionOp>(acceptOp, ...)?

lib/Transforms/InlineParsers.cpp Outdated

+                          }
+                      }
+                      // Move inlined operation `op` to its desired position.

Collaborator

asl Oct 26, 2025

I would probably suggest to expand the comment specifying the IR before this function (e..g how the states were split and where everything went into) and what should be do

Collaborator Author

mtsamis Oct 27, 2025

Good point, I'll add more details.

mtsamis added 4 commits

October 29, 2025 18:51


          Add new helpers with IRUtils.h

0c1149e

Signed-off-by: Manolis Tsamis <[email protected]>


          Add new helper class SplitStateRewriter

9f04327

Signed-off-by: Manolis Tsamis <[email protected]>


          Add ApplyOp::getInstantiateOp, InstantiateOp::getCalleeParser helpers

bf8d22c

Signed-off-by: Manolis Tsamis <[email protected]>


          Add new optimization pass InlineParsers

6e60de6

Signed-off-by: Manolis Tsamis <[email protected]>

mtsamis force-pushed the mtsamis/inline-subparsers-new branch from 20299e1 to 6e60de6 Compare

October 29, 2025 18:29

Collaborator Author

mtsamis commented Oct 29, 2025

In latest version:

Fix bug where updateNamesAndRefs symbol ref rewriting would not take into account if the root parser op matches the callee parser.
Iteratively inline subparsers until nothing new is inlined so we don't need to assume P4 parser order. A new test is also added which contains different parser order than what expected.
Use the location of the apply that failed inlining in notifyMatchFailure, so it's easier to track down the reason that a subparser was not inlined.
Preserve placement of inlined instantiations and variables with respect to the caller's top-level operations.
Update correct version of IRUtils, which includes the improvements that are part of Implement new remove-parser-control-flow optimization pass (#99) #221
Simplify some code, add some more comments.
Address all other review feedback.

mtsamis mentioned this pull request

Implement parser and control inlining #246

Merged

Collaborator Author

mtsamis commented Nov 7, 2025

Superseded by #246.

mtsamis closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet