Implement select cases simplifications (#204) by mtsamis · Pull Request #217 · p4lang/p4mlir-incubator

mtsamis · 2025-07-29T18:07:55Z

This PR implements the various transition_select optimizations from ticket #204.

Four of the five transformations (SimplifySelectList / ReplaceSelectRange / RemoveSelectBooleans / SingleArgumentSelect) are implemented as patterns of a new SimplifySelect optimization pass, while the equivalent of SimplifySelectCases is implemented as ParserTransitionSelectOp canonicalizations.

To aid implementing these optimizations and also simplify the IR, there is an initial commit that makes p4hir.transition_select and the corresponding yield statements have variadic arguments. This was done because having either a single argument or a tuple of arguments and set products made all passes more complicated and required code duplication to handle one vs many arguments.

There is also an additional commit that refactors CastOp constant folding and fixes various signedness bugs around it.

asl

See comments

asl · 2025-07-30T21:42:37Z

test/Transforms/Folds/select.mlir

+
+    // CHECK-COUNT-2: p4hir.select_case
+
+    p4hir.state @start {


I would probably suggest to do CHECK-LABEL: p4hir.state @start to make checks more structured. Same for other cases. You won't need CHECK-COUNT above then.

Thanks for pointing out, done.
(I kept the check-count because it was meant to be only for the first state, not both)

asl · 2025-07-30T21:43:25Z

test/Transforms/Passes/simplify-select-concat-args.mlir

+    %set_3 = p4hir.const #set_const_of_int2_i8i
+    %set_4 = p4hir.const #set_const_of_int1_b2i
+    %everything = p4hir.const #everything
+    p4hir.state @start {


See above. I'd use CHECK-LABEL to narrow the check place

asl · 2025-07-30T21:43:45Z

test/Transforms/Passes/simplify-select-flatten-tuples.mlir

+    %false = p4hir.const #false
+    %w = p4hir.variable ["w", init] : <!p4hir.bool>
+    p4hir.assign %false, %w : <!p4hir.bool>
+    p4hir.state @start {


asl · 2025-07-30T21:44:13Z

test/Transforms/Passes/simplify-select-replace-bools.mlir

+    %false = p4hir.const #false
+    %w = p4hir.variable ["w", init] : <!p4hir.bool>
+    p4hir.assign %false, %w : <!p4hir.bool>
+    p4hir.state @start {


asl · 2025-07-30T21:47:31Z

lib/Transforms/SimplifySelect.cpp

+
+    // Collect operations to apply patterns.
+    llvm::SmallVector<mlir::Operation *, 16> ops;
+    getOperation()->walk<mlir::WalkOrder::PostOrder>([&](mlir::Operation *op) {


I think you can just do

getOperation()->walk<mlir::WalkOrder::PostOrder>([&](P4HIR::ParserTransitionSelectOp op) { ops.push_back(op); }

What is the purpose of postorder traversal?

Even more, it seems we can explicitly apply to the ops, right? Something along https://mlir.llvm.org/docs/PassManagement/#operation-pass-static-filtering-by-op-type and corresponding tablegen bits.

Yes, thanks for pointing out, I copied this from another pass and didn't though about it, fixed now:

Modified pass in tablegen to only apply on ParserOp.

Removed WalkOrder and mlir::isa bits.

Added strictMode = mlir::GreedyRewriteStrictness::ExistingOps config to avoid unnecessary iterations on other ops.

asl · 2025-07-30T22:06:22Z

lib/Transforms/SimplifySelect.cpp

+            return false;
+
+        bool isShapePrimitive = (shape.size() == 1)
+                                && !shape.front().getDefiningOp<P4HIR::TupleOp>();


is this really how clang-format did? If not, will you please reformat the things? :)

No, because I didn't use clang-format 😅
I now properly formatted all the code in the last push.

asl · 2025-08-01T05:56:28Z

lib/Transforms/SimplifySelect.cpp

+    getOperation()->walk([&](P4HIR::ParserTransitionSelectOp op) { ops.push_back(op); });
+
+    mlir::GreedyRewriteConfig config;
+    config.strictMode = mlir::GreedyRewriteStrictness::ExistingOps;


What if we'd have bool inside tuple as a key? Won't this prevent application of second pattern to the newly created op? Should we use ExistingAndNewOps here?

It's always the same transition select op and we just modify the arguments / cases. So no, it doesn't prevent application of the pattern multiple times.

asl

See question above

mtsamis · 2025-08-01T14:05:12Z

Latest push fixes and improves the code for FlattenTuples:

Previously both shape and keys arguments in flattenValues were mlir::ValueRange and we unpacked shape by looking for TupleOp. This was incorrect, since a tuple can be something other than a TupleOp (e.g. a tuple valued argument). Apart from resulting in bugs it also didn't make sense for shape to be a ValueRange.

In the latest code shape is replaced by the appropriate TypeRange and we unpack the actual type. The function now creates p4hir.tuple_extract operations to unpack values of tuple type that are not the result of a TupleOp.

The test case was extended with the previously failing cases.

asl · 2025-08-01T15:43:38Z

In the latest code shape is replaced by the appropriate TypeRange and we unpack the actual type. The function now creates p4hir.tuple_extract operations to unpack values of tuple type that are not the result of a TupleOp.

Can you always do this? And let canonicalizer fold the extracts?

mtsamis · 2025-08-05T13:02:21Z

In the latest code shape is replaced by the appropriate TypeRange and we unpack the actual type. The function now creates p4hir.tuple_extract operations to unpack values of tuple type that are not the result of a TupleOp.

Can you always do this? And let canonicalizer fold the extracts?

Done by also implementing TupleExtractOp::fold (same with StructExtractOp).

mtsamis · 2025-08-05T13:05:37Z

The last force-pushed revision of this also modified the SetAttr::getConstantValue.
Instead of a templated AttrT type it handles all Attr types and returns the appropriate constant value:

llvm::APInt getConstantValue() {
  assert(getKind() == SetKind::Constant && "Expected Constant kind set.");
  assert(getMembers().size() == 1 && "Expected single member set.");
  auto attr = getMembers()[0];
  if (auto intAttr = mlir::dyn_cast<P4HIR::IntAttr>(attr)) {
    return intAttr.getValue();
  } else if (auto boolAttr = mlir::dyn_cast<P4HIR::BoolAttr>(attr)) {
    return llvm::APInt(1, static_cast<uint64_t>(boolAttr.getValue()));
  } else {
    auto enumFieldAttr = mlir::cast<P4HIR::EnumFieldAttr>(attr);
    auto enumType = mlir::cast<P4HIR::SerEnumType>(enumFieldAttr.getType());
    auto field = enumFieldAttr.getField().getValue();
    return enumType.valueOf<P4HIR::IntAttr>(field).getValue();
  }
}

This fixed a crash when we had a SerEnum type in a select during the MakeSingleArgument pattern.

Signed-off-by: Manolis Tsamis <tsamismanolis@gmail.com>

…f SimplifySelectCases Signed-off-by: Manolis Tsamis <tsamismanolis@gmail.com>

mtsamis · 2025-08-18T14:10:49Z

Changes in the last version:

Introduce new helper P4HIR::getConstantInt to convert an mlir::Attribute to llvm::APSInt when applicable.
foldCastConstant now fixes another upstream bug that resulted in a crash when casting a CtorParamAttr constant.
Add SetAttr::isEmptyRange to check for empty ranges (ranges [min, max] where min > max) and fix handling of empty ranges in ReplaceRanges.
Fix handling of select arguments that are not BitsType in ReplaceRanges (e.g. SerEnumType).
Fix mask types and improve code in MakeSingleArgument::createMaskOp

With these changes all samples from the P4C repository transform without crashes.

asl · 2025-08-21T02:04:31Z

lib/Transforms/SimplifySelect.cpp

+
+ private:
+    // Create appropriate casts to convert `keyset` to a set with element type `newType`.
+    mlir::Value castKeysetValue(mlir::PatternRewriter &rewriter, mlir::Type newType,


Can we utilize TypeConverter here? We're having bunch of helpers in p4mlir/Conversion/ConversionPatterns.h

asl self-requested a review July 30, 2025 21:35

asl linked an issue Jul 30, 2025 that may be closed by this pull request

Add select cases simplification #204

Open

asl reviewed Jul 30, 2025

View reviewed changes

mtsamis force-pushed the mtsamis/transition-select-opt branch 2 times, most recently from 30dc9dc to 3eb26e9 Compare July 31, 2025 09:12

asl reviewed Aug 1, 2025

View reviewed changes

mtsamis force-pushed the mtsamis/transition-select-opt branch from 3eb26e9 to 97acab0 Compare August 1, 2025 13:58

mtsamis force-pushed the mtsamis/transition-select-opt branch from 97acab0 to 752096c Compare August 5, 2025 12:59

mtsamis added 4 commits August 18, 2025 15:54

Use variadic arguments instead of tuples for p4hir.transition_select

9b0fb01

Signed-off-by: Manolis Tsamis <tsamismanolis@gmail.com>

Introduce getConstantInt, foldCastConstant and fix cast folding bugs

860d7e5

Signed-off-by: Manolis Tsamis <tsamismanolis@gmail.com>

Add new pass SimplifySelect and implement optimization SelectFlattening

8a104db

Signed-off-by: Manolis Tsamis <tsamismanolis@gmail.com>

Add new SimplifySelect optimization ReplaceBoolWithInt

679f512

Signed-off-by: Manolis Tsamis <tsamismanolis@gmail.com>

mtsamis force-pushed the mtsamis/transition-select-opt branch from 752096c to 92e6a98 Compare August 18, 2025 13:56

mtsamis added 3 commits August 18, 2025 17:10

Add new SimplifySelect optimization ReplaceRanges

b0c6954

Signed-off-by: Manolis Tsamis <tsamismanolis@gmail.com>

Add new SimplifySelect optimization MakeSingleArgument

74bb207

Signed-off-by: Manolis Tsamis <tsamismanolis@gmail.com>

Add ParserTransitionSelectOp::canonicalize and implement equivalent o…

7337dfe

…f SimplifySelectCases Signed-off-by: Manolis Tsamis <tsamismanolis@gmail.com>

mtsamis force-pushed the mtsamis/transition-select-opt branch from 92e6a98 to 7337dfe Compare August 18, 2025 14:10

asl reviewed Aug 21, 2025

View reviewed changes

PietroGhg mentioned this pull request Nov 20, 2025

Add initial implementation for BMv2 headers and parsers #272

Merged

Conversation

mtsamis commented Jul 29, 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!

mtsamis Jul 31, 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!

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!

asl left a comment

Choose a reason for hiding this comment

Uh oh!

mtsamis commented Aug 1, 2025

Uh oh!

asl commented Aug 1, 2025

Uh oh!

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

Uh oh!

Uh oh!

mtsamis commented Aug 5, 2025

Uh oh!

mtsamis commented Aug 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mtsamis Jul 31, 2025 •

edited

Loading

mtsamis commented Aug 5, 2025 •

edited

Loading