Structs and Headers flattening pass by PietroGhg · Pull Request #309 · p4lang/p4mlir-incubator

PietroGhg · 2026-01-28T19:10:57Z

Adds a FlattenStructs pass that ensures that P4HIR Structs only contain Headers or scalars, and that Headers only contain scalars.
Addresses #251

lib/Transforms/FlattenStructs.cpp

mtsamis

Thanks for the PR, please see some comments for potential improvements.

mtsamis · 2026-02-02T17:09:26Z

lib/Transforms/FlattenStructs.cpp

+
+        target.addDynamicallyLegalOp<P4HIR::StructExtractOp>([&](P4HIR::StructExtractOp op) {
+            return typeConverter.isLegal(op.getInput().getType());
+        });


I assume there is some reason that P4HIR::StructFieldRefOp and P4HIR::StructExtractOp should be explicitly defined in addition to configureUnknownOpDynamicallyLegalByTypes? If so, maybe a comment would be good to explain.

Since the conversion pattern I added merges together a sequence of struct access ops, I wanted to only declare the "root" operations of a struct access chain as illegal. But given that the conversion happens top-down, the root operation will be the first to match so we can just remove these lines of code

mtsamis · 2026-02-02T17:18:47Z

lib/Transforms/FlattenStructs.cpp

+            isPredicateOrRefToPredicate(resultTy, StructFlatteningTypeConverter::isHeader) ||
+            isPredicateOrRefToPredicate(resultTy, StructFlatteningTypeConverter::isScalar) ||
+            isPredicateOrRefToPredicate(resultTy,
+                                        [](Type ty) { return isa<P4HIR::ValidBitType>(ty); });


I think isPredicateOrRefToPredicate doesn't provide a lot of utility or readability here, maybe better to do something like below?

mlir::Type unwrapRef = leafType; if (auto refType = dyn_cast<P4HIR::ReferenceType>(leafType)) leafType = refType.getObjectType(); bool isLeaf = StructFlatteningTypeConverter::isHeader(leafType) || StructFlatteningTypeConverter::isScalar(leafType) || isa<P4HIR::ValidBitType>(leafType);

mtsamis · 2026-02-02T17:24:47Z

lib/Transforms/FlattenStructs.cpp

+                                          ConversionPatternRewriter &rewriter) const {
+        return llvm::TypeSwitch<Operation *, LogicalResult>(op)
+            .Case([&](P4HIR::StructFieldRefOp fieldRefOp) {
+                return processStructAccessOp(fieldRefOp, convertedInput, parentFieldPath, eraseList,


If you want to avoid some duplication here you can use this pattern:

.Case<P4HIR::StructFieldRefOp, P4HIR::StructExtractOp>([&](auto op) { return processStructAccessOp(fieldRefOp, convertedInput, parentFieldPath, eraseList, replacements, rewriter); }

mtsamis · 2026-02-02T17:30:02Z

lib/Transforms/FlattenStructs.cpp

+            bool outputIsRef = isa<P4HIR::ReferenceType>(op.getResult().getType());
+            Value newResult =
+                llvm::TypeSwitch<Type, Value>(convertedInput.getType())
+                    .Case<P4HIR::ReferenceType>([&](auto) {


This is a bit hard to read due to the identation. I would propose that TypeSwitch doesn't provide a lot of benefit since there's only one case and you could use something like

Value newResult; if (mlir::isa<P4HIR::ReferenceType>(convertedInput.getType())) { ... newResult = ...; } else { newResult = rewriter.create(...); }

That should eliminate 3 identation levels and make it easier to read.

mtsamis · 2026-02-02T17:30:47Z

lib/Transforms/FlattenStructs.cpp

+        // For intermediate reads we just process the rest of the tree and add the read to the erase
+        // list
+        for (auto user : op.getResult().getUsers()) {
+            if (!isa<P4HIR::StructFieldRefOp, P4HIR::StructExtractOp>(user)) {


Remove brackets for consistency with if below.

mtsamis · 2026-02-02T17:33:49Z

lib/Transforms/FlattenStructs.cpp

+
+        populateFunctionOpInterfaceTypeConversionPattern<P4HIR::FuncOp>(patterns, typeConverter);
+
+        if (failed(applyPartialConversion(moduleOp, target, std::move(patterns)))) {


Remove brackets 🙂

mtsamis · 2026-02-02T17:34:08Z

lib/Transforms/FlattenStructs.cpp

+        auto moduleOp = getOperation();
+
+        StructFlatteningTypeConverter typeConverter;
+


Can remove empty line here.

mtsamis · 2026-02-02T17:35:32Z

lib/Transforms/FlattenStructs.cpp

+class StructFlatteningTypeConverter : public P4HIRTypeConverter {
+ public:
+    StructFlatteningTypeConverter() {
+        addConversion([](P4HIR::StructLikeTypeInterface type) -> std::optional<Type> {


-> std::optional<Type> shouldn't really be needed here since flattenStructLikeType doesn't return one.

mtsamis · 2026-02-02T17:38:24Z

lib/Transforms/FlattenStructs.cpp

+                continue;
+            } else {
+                llvm::errs() << "Unexpected type " << fieldType << "\n";
+                llvm_unreachable("Unexpected field type during flattening");


llvm::errs + llvm_unreachable feels a bit strange here. Consider that due to llvm_unreachable there (probably) won't be any message printed in a release build, if that case was ever hit.

So, either use llvm_unreachable only with that message, or keep llvm::errs() and don't use unreachable. (I would propose to only keep llvm_unreachable, if this indeed should be unreachable).

Yeah the errs was left over from debugging, thanks for spotting it. I've removed it

mtsamis · 2026-02-02T17:40:48Z

test/Transforms/Passes/flatten-structs.mlir

+// CHECK-SAME:      %[[ARG0:.*]]: !p4hir.ref<!metadata>,
+// CHECK-SAME:      %[[ARG1:.*]]: !p4hir.ref<!b16i>,
+// CHECK-SAME:      %[[ARG2:.*]]: !p4hir.ref<!b16i>) {
+// CHECK:           %[[VAL_0:.*]] = p4hir.read %[[ARG0]] : <!metadata>


Minor thing, but I think it would be better to use CHECK-DAG here, and in the other functions matched.

Signed-off-by: Pietro Ghiglio <pghiglio@accesssoftek.com>

mtsamis

Looks good!

asl self-requested a review January 30, 2026 06:56

asl reviewed Jan 30, 2026

View reviewed changes

lib/Transforms/FlattenStructs.cpp Outdated Show resolved Hide resolved

PietroGhg force-pushed the pietro/flatten_struct branch 2 times, most recently from 6751be6 to 332f9af Compare January 30, 2026 08:15

mtsamis requested changes Feb 2, 2026

View reviewed changes

Structs and Headers flattening pass

7784f46

Signed-off-by: Pietro Ghiglio <pghiglio@accesssoftek.com>

PietroGhg force-pushed the pietro/flatten_struct branch from 332f9af to 7784f46 Compare February 3, 2026 12:58

mtsamis approved these changes Feb 9, 2026

View reviewed changes


		populateFunctionOpInterfaceTypeConversionPattern<P4HIR::FuncOp>(patterns, typeConverter);

		if (failed(applyPartialConversion(moduleOp, target, std::move(patterns)))) {

		auto moduleOp = getOperation();

		StructFlatteningTypeConverter typeConverter;

Conversation

PietroGhg commented Jan 28, 2026

Uh oh!

Uh oh!

mtsamis 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants