Implement header union type and header union type isValid built-in method #96
Conversation
…thod. Signed-off-by: Vladimir Shiryaev <w@tagolog.com>
| !p4hir.header_union<"name", field1: HeaderType1, field2: HeaderType2> | ||
| }]; | ||
|
|
||
| // We skip default builders entirely to consistently add validity bit field on fly |
There was a problem hiding this comment.
Do we really need this? One can construct union from headers only that already contain validity bits.
| @@ -264,6 +300,12 @@ HeaderType HeaderType::get(mlir::MLIRContext *context, llvm::StringRef name, | |||
| return Base::get(context, name, realFields); | |||
| } | |||
|
|
|||
| HeaderUnionType HeaderUnionType::get(mlir::MLIRContext *context, llvm::StringRef name, | |||
| llvm::ArrayRef<FieldInfo> fields) { | |||
| llvm::SmallVector<FieldInfo, 4> realFields(fields); | |||
There was a problem hiding this comment.
This looks strange. Why copy fields?
|
|
||
| action header_union_isValid_test() { | ||
| U u; | ||
| if (u.isValid()) |
There was a problem hiding this comment.
You also need tests for # literal as well as setValid / setInvalid, etc. calls
| // CHECK: %[[VAL_4:.*]] = p4hir.read %[[VAL_3]] : <!validity_bit> | ||
| // CHECK: %[[VAL_5:.*]] = p4hir.const #[[$ATTR_2]] | ||
| // CHECK: %[[VAL_6:.*]] = p4hir.cmp(eq, %[[VAL_4]], %[[VAL_5]]) : !validity_bit, !p4hir.bool | ||
| // CHECK: %[[VAL_7:.*]] = p4hir.ternary(%[[VAL_1]], true { |
There was a problem hiding this comment.
The generated code looks a bit weird, as we're evaluating all nested isValid's even if we already deduced that the the result is true. I think we'd need to shortcut and emit something like this:
(H.h1.isValid() ? true : (H.h2.isValid() ? true : (H.h3.isValid() ? : ...))). So, it will be a sequence of nested ternary operators where you're yielding true in true region and recurse in false region.
| @@ -329,6 +330,10 @@ class P4HIRConverter : public P4::Inspector, public P4::ResolutionContext { | |||
| // Should be resolved eslewhere | |||
| return false; | |||
| } | |||
| bool preorder(const P4::IR::InvalidHeaderUnion *hu) override { | |||
There was a problem hiding this comment.
We need test for this (# literal).
| @@ -1276,7 +1360,13 @@ void P4HIRConverter::postorder(const P4::IR::Member *m) { | |||
| // TODO: Likely we can do similar things for the majority of struct-like | |||
| // types | |||
| auto parentType = getOrCreateType(m->expr); | |||
| if (mlir::isa<P4HIR::StructType, P4HIR::HeaderType>(parentType)) { | |||
| if (mlir::isa<P4HIR::StructType, P4HIR::HeaderUnionType>(parentType)) { | |||
There was a problem hiding this comment.
why duplicate the code? Just add another case to isa
| @@ -1139,21 +1224,20 @@ bool P4HIRConverter::preorder(const P4::IR::MethodCallExpression *mce) { | |||
| // TODO: Are there cases when we do not have l-value here? | |||
| auto ref = resolveReference(bCall->appliedTo); | |||
|
|
||
| mlir::Value P4HIRConverter::emitHeaderUnionBuiltInMethod( | ||
| const P4::IR::Type_HeaderUnion *p4cHeaderUnionType, const P4::BuiltInMethod *builtInMethod, | ||
| mlir::OpBuilder &builder, mlir::Location loc) { |
There was a problem hiding this comment.
no need to pass builder here (and above).
|
|
||
| for (auto huField : huFields) { | ||
| auto headerRef = builder.create<P4HIR::StructExtractRefOp>(loc, headerUnion, huField.name); | ||
| auto validityBitRef = builder.create<P4HIR::StructExtractRefOp>( |
There was a problem hiding this comment.
Maybe you can refactor the code a bit, so you could reuse some pieces of emitHeaderBuiltInMethod?
E.g.:
- Instead of passing
builtInMethod, pass only method name - Instead of emitting a reference to a header (via
resolveReference), inside do it outside, and pass themlir::Valueas argument.
This way you'd essentially need to call such refactored emitHeaderBuiltInMethod with headerRef argument and then combine the resulting values properly.
| auto validityBit = builder.create<P4HIR::ReadOp>(loc, validityBitRef); | ||
| auto isHeaderValid = builder.create<P4HIR::CmpOp>(loc, P4HIR::CmpOpKind::Eq, validityBit, | ||
| getValidHeaderConstant(loc)); | ||
| auto ternaryOpResult = builder.create<P4HIR::TernaryOp>( |
There was a problem hiding this comment.
See above about expected code sequence.
Signed-off-by: Vladimir Shiryaev w@tagolog.com