Implement new remove-parser-control-flow optimization pass (#99) #221

mtsamis · 2025-08-18T14:30:42Z

This PR implements the equivalent of P4C's RemoveParserControlFlow pass. It replaces if and switch statements inside parser states with newly introduced states and transitions.

Some infrastructure for this pass is generally useful and will be helpful in implementing subparser/control inlining. As such, it has been placed in a separate IRUtils namespace.

(This is very similar to #220, which I accidentally closed by deleting the branch. This new branch contains few more helper functions and a different TransitionSelectBuilder::addCases implementation).

asl

See comments (mostly minor) here

asl · 2025-08-20T17:19:21Z

include/p4mlir/Transforms/IRUtils.h

+
+// Helper to create a new empty sub-state for `state`.
+P4HIR::ParserStateOp createSubState(mlir::RewriterBase &rewriter, P4HIR::ParserStateOp state,
+                                    llvm::StringRef suffix);


Use Twine for suffix.

asl · 2025-08-20T17:22:56Z

include/p4mlir/Dialect/P4HIR/P4HIR_ParserOps.td

-    auto getNextTransition() { return getBody().back().getTerminator(); }
+    auto getNextTransition() { return getBlock()->getTerminator(); }
+
+    /// Get a SymbolRefAttr for this parser state.


Can we be a bit more specific here. Essentially it is a fully qualified state name, right? So this should be explained here :)

asl · 2025-08-20T19:13:38Z

include/p4mlir/Transforms/IRUtils.h

+// Inline `scopeOp`'s body to its parent.
+void inlineScope(mlir::RewriterBase &rewriter, P4HIR::ScopeOp scopeOp);
+
+// Return true if it's valid to call `splitBlockAt` for the given arguments.


It would help to explain better the invariants / conditions here.

asl · 2025-08-20T19:22:49Z

lib/Transforms/IRUtils.cpp

+    // Split resulting block.
+    mlir::Block *before = op->getBlock();
+    mlir::Block *middle = rewriter.splitBlock(before, op->getIterator());
+    mlir::Block *after = rewriter.splitBlock(middle, ++op->getIterator());


Suggested change

mlir::Block *after = rewriter.splitBlock(middle, ++op->getIterator());

mlir::Block *after = rewriter.splitBlock(middle, std::next(op->getIterator()));

asl · 2025-08-20T19:39:58Z

lib/Transforms/IRUtils.cpp

+    // This is needed to properly handle operands after `copyOp` calls.
+    for (mlir::Operation &op : llvm::make_early_inc_range(llvm::reverse(*block))) {
+        if (mlir::isa<P4HIR::VariableOp>(op)) {
+            // Promote variables to a parser locals.


Suggested change

// Promote variables to a parser locals.

// Move variables to top-level. Rewriter position is expected to point where we can create such locals (e.g. parser or control locals

Note that for control locals we'd need to be a bit more elaborated as actions are isolated from above, so they are exported via "symbol references"

asl · 2025-08-20T19:47:55Z

lib/Transforms/IRUtils.cpp

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


Can you ask symbol table to generate unique symbol for you?

E.g.:

auto getUniqueName = [&](mlir::StringAttr toRename) { unsigned counter = 0; return mlir::SymbolTable::generateSymbolName<256>( toRename, [&](llvm::StringRef candidate) { return parser.lookupSymbol(StringAttr::get(candidate)) != nullptr; }, counter); }; auto nameAttr = getUniqueName(StringAttr::get(llvm::Twine(state.getSymName()) + "_" + suffix)));

You might want to tune strings types here though :)

asl · 2025-08-20T19:50:55Z

include/p4mlir/Transforms/IRUtils.h

+
+    // Create new intermediate state that transitions to `transitionTo` and move `ops` in it.
+    // If `ops` is non-null the terminator of the block is erased once moved.
+    P4HIR::ParserStateOp createSubState(llvm::StringRef suffix,


Use twines here and below

mtsamis · 2025-08-21T10:23:12Z

Thanks for the review! I have addressed your comments in the latest version.

In addition, I made adjustBlockUses to only move variables if they escape, and simplified ParserStateOp::getSymbolRef by using more appropriate get functions.

Note that for control locals we'd need to be a bit more elaborated as actions are isolated from above, so they are exported via "symbol references"

Indeed, the current version probably won't work for control locals. It looks like it would need some work to get them right, but we don't currently need that somewhere. Should we add an assertion for now and implement in the future if needed?

asl · 2025-08-21T15:31:36Z

Indeed, the current version probably won't work for control locals. It looks like it would need some work to get them right, but we don't currently need that somewhere. Should we add an assertion for now and implement in the future if needed?

Yeah, let's do an assert for now.

mtsamis · 2025-08-21T16:23:31Z

Added assert for ControlOp in adjustBlockUses

asl · 2025-08-21T17:25:52Z

include/p4mlir/Transforms/IRUtils.h

+
+    // Create new intermediate state that transitions to `transitionTo` and move `ops` in it.
+    // If `ops` is non-null the terminator of the block is erased once moved.
+    P4HIR::ParserStateOp createSubState(llvm::Twine suffix, P4HIR::ParserStateOp transitionTo = {},


This is a common problem :)

Twines should only be used as const references in arguments

See https://github.com/llvm/llvm-project/blob/bce9b6d1771bbcf6d250935fdaab1dfa0922fd72/llvm/include/llvm/ADT/Twine.h#L38

I see, thanks for pointing out. I force pushed the fix.

asl · 2025-09-23T06:46:47Z

@mtsamis Looks like it does not compile anymore

mtsamis · 2025-09-23T06:48:53Z

@mtsamis Looks like it does not compile anymore

Thanks for reporting, I'll have a look.

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

mtsamis force-pushed the mtsamis/remove-parser-cf branch from ceda822 to f2e7092 Compare August 20, 2025 11:02

asl force-pushed the mtsamis/remove-parser-cf branch from f2e7092 to cab6581 Compare August 20, 2025 17:15

asl self-requested a review August 20, 2025 17:15

asl linked an issue Aug 20, 2025 that may be closed by this pull request

Remove conditional control flow from parsers #99

Open

asl reviewed Aug 20, 2025

View reviewed changes

mtsamis force-pushed the mtsamis/remove-parser-cf branch from cab6581 to e6c0ed9 Compare August 21, 2025 10:14

mtsamis force-pushed the mtsamis/remove-parser-cf branch from e6c0ed9 to 55a6e4e Compare August 21, 2025 16:23

asl reviewed Aug 21, 2025

View reviewed changes

mtsamis force-pushed the mtsamis/remove-parser-cf branch from 55a6e4e to 9f491a8 Compare August 22, 2025 13:45

asl force-pushed the mtsamis/remove-parser-cf branch 2 times, most recently from 0b4b930 to 596ac64 Compare September 23, 2025 06:46

mtsamis added 5 commits October 2, 2025 00:00

Add new helpers with IRUtils.h

3bb6139

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

Add new helper class SplitStateRewriter

0216029

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

Add new helper class TransitionSelectBuilder

0bb5e2a

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

Add new optimization pass RemoveParserControlFlow

c9eb6ce

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

Fix compilation error due to upstream MLIR changes

e29f8c5

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

asl force-pushed the mtsamis/remove-parser-cf branch from 2671cb6 to e29f8c5 Compare October 1, 2025 22:00

mtsamis mentioned this pull request Oct 29, 2025

Implement parser inlining #241

Closed

	mlir::Block *after = rewriter.splitBlock(middle, ++op->getIterator());
	mlir::Block *after = rewriter.splitBlock(middle, std::next(op->getIterator()));

	// Promote variables to a parser locals.
	// Move variables to top-level. Rewriter position is expected to point where we can create such locals (e.g. parser or control locals

Implement new remove-parser-control-flow optimization pass (#99) #221

Are you sure you want to change the base?

Implement new remove-parser-control-flow optimization pass (#99) #221

Uh oh!

Conversation

mtsamis commented Aug 18, 2025

Uh oh!

asl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtsamis commented Aug 21, 2025

Uh oh!

asl commented Aug 21, 2025

Uh oh!

mtsamis commented Aug 21, 2025

Uh oh!

asl Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asl commented Sep 23, 2025

Uh oh!

mtsamis commented Sep 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

asl Aug 21, 2025 •

edited

Loading