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

[P4Testgen] Flatten the genEq function and support struct expressions. #4225

Merged

fruffy merged 4 commits into main from fruffy/flatten_gen_eq

Nov 23, 2023

Collaborator

fruffy commented Nov 5, 2023 •

edited

Loading

Simplify genEq by performing all checks within a single function and resolving nested expressions recursively. Also ensure support of struct expressions. Note that this assumes that the elements within the struct expression are already ordered, i.e, they are enumerated the same way the underlying type is. The struct expression comparison is also on the basis of the contained expressions. We do not check whether the names are the same.

fruffy added the p4tools label

fruffy force-pushed the fruffy/flatten_gen_eq branch from 8d96a28 to 6bef935 Compare

November 5, 2023 23:33

fruffy marked this pull request as ready for review

November 6, 2023 00:46

fruffy force-pushed the fruffy/flatten_gen_eq branch from 6bef935 to d71aebe Compare

November 13, 2023 04:10

fruffy requested a review from pkotikal

November 13, 2023 22:36

fruffy force-pushed the fruffy/flatten_gen_eq branch from d71aebe to a921d91 Compare

November 13, 2023 22:36

fruffy requested a review from jnfoster

November 15, 2023 13:45

fruffy force-pushed the fruffy/flatten_gen_eq branch from a921d91 to d3de6af Compare

November 15, 2023 13:47

fruffy requested a review from vlstill

November 16, 2023 17:42

Collaborator Author

fruffy commented Nov 16, 2023

@vlstill This PR is in preparation for full struct expression support in P4Testgen. Ultimately, this will allow us to pass struct expressions to functions, which will resolve the problem that we can not have both the reference and the value in extern calls. I have a series of PRs that will implement this.

vlstill reviewed

View reviewed changes

backends/p4tools/common/lib/gen_eq.h Outdated

Comment on lines 21 to 22

		/// Important, this equation assumes that struct expressions have been ordered.
		/// This calculation does not match the names of the struct expressions.

Contributor

vlstill Nov 18, 2023

Should there be some check that the order matches? This seems to me like a possible source of very hard to find bugs.

Collaborator Author

fruffy Nov 18, 2023

This is a little difficult to implement because it might need a lot of special casing. Let me think about something sensible. In general, the assumption is that the front-end has taking care of ordering.

Collaborator Author

fruffy Nov 18, 2023

Actually, the right way to do this is to simply check the type of the underlying struct expression and match it up with the components. Unfortunately, that type is often a Type_Name, very inconvenient...

Contributor

vlstill Nov 21, 2023 •

edited

Loading

Actually, the right way to do this is to simply check the type of the underlying struct expression and match it up with the components. Unfortunately, that type is often a Type_Name, very inconvenient...

Is it not possible to resolve the Type_Name using P4::TypeMap at this point?

Collaborator Author

fruffy Nov 21, 2023

I have implemented this and it seems to pass all tests for now.

Is it not possible to resolve the Type_Name using P4::TypeMap at this point?

We do not have access to the TypeMap at this point, P4Testgen does its own type resolution because passes can break type-checking. Usually, any Type_Name is resolved before. The problem is Type_Names in nested struct expressions. We do not have to deal with this right now because we resolve most nesting.

Collaborator Author

fruffy Nov 21, 2023

Now that #4215 is merged, which resolves Type_Name, this is a much safer assumption and will hold.

Contributor

vlstill Nov 22, 2023

I see. Do you plan to add an additional check, or is this kept as a precondition only? I think this is actually complicated by the eager resolving of singletons which means the types don't need to match exactly. Therefore it is probably OK to just leave the "compatibility" of types as a precondition.

backends/p4tools/common/lib/gen_eq.h Show resolved Hide resolved

backends/p4tools/common/lib/gen_eq.cpp Outdated

+              const IR::Expression *GenEq::checkSingleton(const IR::Expression *expr) {
+                  if (const auto *listExpr = expr->to<IR::BaseListExpression>()) {
+                      if (listExpr->size() == 1) {
+                          expr = checkSingleton(listExpr->components.at(0));

Contributor

vlstill Nov 18, 2023

why not return directly?

backends/p4tools/common/lib/gen_eq.cpp Outdated Show resolved Hide resolved

backends/p4tools/common/lib/gen_eq.cpp Outdated

+                  } else if (auto structExpr = left->to<IR::StructExpression>()) {
+                      leftElems = IR::flattenStructExpression(structExpr);
+                  } else {
+                      BUG("Unsupported list expression %1% of type %2%.", left, left->node_type_name());

Contributor

vlstill Nov 18, 2023

Suggested change

      
                    BUG("Unsupported list expression %1% of type %2%.", left, left->node_type_name());
          
                    BUG("Unsupported list/struct expression %1% of type %2%.", left, left->node_type_name());

or something like that

backends/p4tools/common/lib/gen_eq.cpp Outdated

Comment on lines 35 to 42

+                  std::vector<const IR::Expression *> rightElems;
+                  if (auto listExpr = right->to<IR::BaseListExpression>()) {
+                      rightElems = IR::flattenListExpression(listExpr);
+                  } else if (auto structExpr = right->to<IR::StructExpression>()) {
+                      rightElems = IR::flattenStructExpression(structExpr);
+                  } else {
+                      BUG("Unsupported right list expression %1% of type %2%.", right, right->node_type_name());
                   }

Contributor

vlstill Nov 18, 2023

Please extract this piece of code to a function, it is there twice.

backends/p4tools/common/lib/gen_eq.cpp Outdated

-                  if (const auto *listKey = keyset->to<IR::ListExpression>()) {
-                      return equate(target, listKey);
+              const IR::Expression *GenEq::checkSingleton(const IR::Expression *expr) {

Contributor

vlstill Nov 18, 2023

Please use different name, check* suggests this will return bool, but it returns an expression.

backends/p4tools/common/lib/gen_eq.cpp Outdated

+                  // If we still have lists after unrolling, compare them.
+                  if (left->is<IR::BaseListExpression>() || left->is<IR::StructExpression>()) {
+                      BUG_CHECK(right->is<IR::BaseListExpression>() || right->is<IR::StructExpression>(),
+                                "Right expression must be a list expression. Is %1% of type %2%.", right,

Contributor

vlstill Nov 18, 2023

Suggested change

      
                              "Right expression must be a list expression. Is %1% of type %2%.", right,
          
                              "Right expression must be a list or struct expression. Is %1% of type %2%.", right,

fruffy force-pushed the fruffy/flatten_gen_eq branch 2 times, most recently from be29f5a to 3ffd79a Compare

November 18, 2023 20:32

fruffy commented

View reviewed changes

ir/irutils.cpp Outdated

               std::vector<const Expression *> flattenStructExpression(const StructExpression *structExpr) {
                   std::vector<const Expression *> exprList;
-                  for (const auto *listElem : structExpr->components) {
+                  const auto *structType = structExpr->type->checkedTo<IR::Type_StructLike>();

Collaborator Author

fruffy Nov 18, 2023

This is a little problematic because the structType can be Type_Name. I am not sure about a better way to handle this, however. At least we throw a bug in this case, which is easy to debug.

For P4Testgen, this is easy to fix once #4215 is approved and merged. We can just add a pass that converts every Type_Name of a struct expression into a Type_Structlike. We should do this anyway.

fruffy force-pushed the fruffy/flatten_gen_eq branch from 3ffd79a to 69e2e2e Compare

November 19, 2023 18:19

fruffy mentioned this pull request

[P4Testgen] Add a compiler pass to resolve Type_Name in StructExpressions. #4215

Merged

fruffy requested a review from vlstill

November 21, 2023 02:55

fruffy force-pushed the fruffy/flatten_gen_eq branch from 69e2e2e to 2cfdf4b Compare

November 21, 2023 23:04

vlstill reviewed

View reviewed changes

Contributor

vlstill left a comment

I can almost approve this now.

backends/p4tools/common/lib/gen_eq.h Outdated

Comment on lines 8 to 11

    
              /// Generates an equality on a target expression and a keyset expression, recursing into lists.

              /// Generates an semantic equality on two input expressions, recursing into lists and structs.

              /// This supports fuzzy matching on singleton lists: singleton lists are considered the same as

              /// their singleton elements. This is implemented by eagerly recursing into singleton lists before

              /// attempting to generate the equality.

Contributor

vlstill Nov 22, 2023

Please mention a precondition that the types need to be compatible.

Also, I suggest you move the documentation to the function and better yet that you make equate into a freestanding function and remove the class altogether (moving the helpers into .cpp).

backends/p4tools/common/lib/gen_eq.h Outdated

Comment on lines 21 to 22

		/// Important, this equation assumes that struct expressions have been ordered.
		/// This calculation does not match the names of the struct expressions.

Contributor

vlstill Nov 22, 2023

I see. Do you plan to add an additional check, or is this kept as a precondition only? I think this is actually complicated by the eager resolving of singletons which means the types don't need to match exactly. Therefore it is probably OK to just leave the "compatibility" of types as a precondition.

ir/irutils.cpp Outdated

+                  // Ensure that the underlying type is a Type_StructLike.
+                  // TODO: How do fail gracefully if we get a Type_Name?
+                  const auto *structType = structExpr->type->to<IR::Type_StructLike>();
+                  BUG_CHECK(structType != nullptr, "%1%: expected a struct type, received %2%", structExpr->type,

Contributor

vlstill Nov 22, 2023

... struct like?

fruffy force-pushed the fruffy/flatten_gen_eq branch from 2cfdf4b to cf7f0f4 Compare

November 22, 2023 13:53

fruffy added 4 commits

November 22, 2023 08:53


          Flatten the genEq function and support struct expressions.

2203b8c


          Review comments.

b316f05


          When flattening a struct expression, use the underlying type, not the…

8f349d0

… components.


          Review comments.

a73efad

fruffy force-pushed the fruffy/flatten_gen_eq branch from cf7f0f4 to a73efad Compare

November 22, 2023 13:53

fruffy requested a review from vlstill

November 22, 2023 22:00

vlstill approved these changes

View reviewed changes

fruffy merged commit 02f4823 into main

13 checks passed

fruffy deleted the fruffy/flatten_gen_eq branch

November 23, 2023 18:29

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

Labels

p4tools