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

Fix for issue #1478 #1503

Merged
merged 2 commits into from
Sep 21, 2018
Merged

Fix for issue #1478 #1503

merged 2 commits into from
Sep 21, 2018

Conversation

mihaibudiu
Copy link
Contributor

Unfortunately the stderr is produced by p4test, and not by p4c-bm2-ss.
But when running p4c-bm2-ss on the sample it will produce two warnings:

issue1478-bmv2.p4(25): warning: 3: Size specified for table without keys
        size = 3;
               ^
issue1478-bmv2.p4(35): warning: 10: Size specified for table with constant entries
        size = 10;
               ^^

Copy link
Member

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

Why not add this to the p4test midend pass manager then?

namespace P4 {

/// Checks some possible misuses of the table size property
class CheckTableSize : public Inspector {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to not make this an inspector and actually replace the size by 0 in that case, so that the backend doesn't have to worry about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not just remove the size property rather than replace it by 0?

Copy link
Member

Choose a reason for hiding this comment

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

I don't really have a preference. It seems that either way each backend has to be careful. Currently the bmv2 backend unconditionally sets the table size to 1024 when the size is missing (or when the provided size is 0), which seems wrong: https://github.com/p4lang/p4c/blob/master/backends/bmv2/common/control.cpp#L386

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that this behavior is inherited from the p4-14 compiler.
The back-end should probably do this only when it is really necessary - i.e., the table size can change dynamically.

Copy link
Member

Choose a reason for hiding this comment

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

bmv2 actually enforced fixed table sizes to emulate hardware targets
the default table size code should probably be updated to account for the key size, at least for exact match tables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then let's file a new issue. Probably @hanw is the right person to handle this, since I believe he has ownership of the bmv2-based backends. For constant tables the compiler should somehow measure the table size and tell bmv2 what that is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the meantime it looks to me like deleting the table size for these two cases is the right thing. The back-end is supposed to measure the table anyway. 0 seems a bit arbitrary.

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.

2 participants