Skip to content
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.

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

Start a list of unsupported P4_16 fatures in bmv2 backend README #564

Merged
merged 3 commits into from
May 4, 2017

Conversation

antoninbas
Copy link
Member

See issue #563
It would be nice if this list could be kept up-to-date as we implement more features in the bmv2 compiler backend or discover more issues. Feel free to update this list if you are aware of more issues.

@antoninbas antoninbas requested review from hanw and mihaibudiu May 3, 2017 21:16
s1_t s02;
};
```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming this is about nested struct used in block constructor parameters? There is already a pass to eliminate nested struct in local variables.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - the BMV2 backend fails if you try to define a metadata or header struct that contains other nested structs. (Or at least, it did last time I tried.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hanw This is related to #562
You may be right, I think this only happens for structs used in block constructor parameters. I can update my PR to be more precise. Any idea why this case is handled differently?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Headers cannot contain nested structs in P4, so only metadata is a problem.
There are no extern blocks in v1model which require structs as parameters, so this is not really an issue with v1model.p4.
There are many bugs filed with are related to unsupported features, including name collisions, and the error type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Headers can't contain nested structs, but it seems to me that we could support nested structs in the "header type" that we instantiate the package with. For v1model.p4, that's the H type parameter to package V1Switch<H,M>. I'd expect to be able to do something like this:

header h1 {...}
struct s1 { h1 h; }
header h2 {...}
struct s2 {h2 h; }

// This type contains nested structs:
struct Headers { s1 nested1; s2 nested2; }

// This should work, but last I checked, it doesn't:
parser p(packet_in b, out Headers h, inout Meta m, inout standard_metadata_t sm) { ... }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possible solution is to flatten the struct to only contain either bit or header type, and then rewrite the parameter list of a block to use the new struct, and update all references to the struct field in the block, table, action, etc, to the new struct field. @mbudiu-vmw Do you other suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of block are we talking about?
We cannot change APIs for externs of objects visible from the control-plane.

For controls and parsers we can do something like that, but it won't be trivial. You also have to cope with operations that may use the sub-structs that you are flattening, so you will have to reconstruct them if needed. If it was simple I would have implemented it by now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about ControlBlock and ParserBlock. I agree that we should not try to transform the parameter list of ExternBlock.

@antoninbas antoninbas force-pushed the bmv2-readme-unsupported-features branch from a0a27d8 to 2328b3c Compare May 3, 2017 22:10
@mihaibudiu
Copy link
Contributor

You should check this in so I can edit it.
Also, having the metadata contain nested structs does not work.


- compound action parameters (can only be `bit<>` or `int<>`)

- functions or methods with a compund return type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas can you please give an example of this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a code snippet

@antoninbas antoninbas force-pushed the bmv2-readme-unsupported-features branch from 2328b3c to 9db52b2 Compare May 4, 2017 01:17
@antoninbas
Copy link
Member Author

I am merging this, since @mbudiu-vmw approved and I think I have addressed all comments. Feel free to update this file if you discover more unsupported features.

@antoninbas antoninbas merged commit a65f22f into master May 4, 2017
@antoninbas antoninbas deleted the bmv2-readme-unsupported-features branch May 4, 2017 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants