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

P4 Compiler must invoke the assembler by default #54

Open
vgurevich opened this issue Jan 27, 2025 · 5 comments
Open

P4 Compiler must invoke the assembler by default #54

vgurevich opened this issue Jan 27, 2025 · 5 comments
Labels
bug Something isn't working

Comments

@vgurevich
Copy link
Contributor

p4lang/p4c#5070 introduced a new command-line parameter designed to prevent the compiler driver from running the Tofino assembler (bfas) by default.

While @fruffy explained the technical reason behind it (bfas is not included in https://github.com/p4lang/p4c), it is critical to always run the assembler when compiling a P4 program for Tofino. Without doing it, it is not even possible to be sure that the program compiled successfully, since it is the assembler that checks that the compiler output can fit into the number of stages that is actually available on the device.

I recommend replacing the flag --enable-bf-asm with --disable-bf-asm and similarly replacing the environment variable ENABLE_BF_ASM with DISABLE_BF_ASM.

I know, this sounds like a compiler issue -- will be happy to move it to p4lang/p4c, but it definitely affects open-p4studio users a lot more.

@jafingerhut
Copy link
Contributor

Unless you know of code changes required in this repo that are relevant, I agree that this seems more like a p4lang/p4c issue.

I guess it is likely that after such a change would be made to p4c, the p4lang/p4c submodule refpoint would need to be updated in order for building this repo's code to take advantage of it.

@vgurevich
Copy link
Contributor Author

@jafingerhut -- we have two options here:

  1. Leave p4lang/p4c code as-is, but add the code in p4studio that will transform the code during the installation
  2. Modify the code in p4lang/p4c

I would prefer the second option, but that requires additional effort. At the very minimum, we need to make sure the changes are integrated into CI. More importantly, I do not understand how we can even maintain the compiler(specifically its Tofino backend) without the assembler. For example, how can we make sure that the code, compiled by it even fits into Tofino's 12 stages. As a result, the proper solution might be quite involved.

@fruffy -- what are your thought? Let's decide on the course of action and then figure out how to get from point A to point B.

@jafingerhut
Copy link
Contributor

jafingerhut commented Jan 28, 2025

If you want CI tests for the Tofino compiler back end, that include the assembler, the three primary choices seem to be:

  1. Develop and run them pre-commit in the p4lang/p4c repository. This requires making the assembler available in p4lang/p4c CI tests, either by having the source for the assembler in that repo, or making binaries stored somewhere, built from p4lang/open-p4studio sources, used during p4lang/p4c CI runs.
  2. Develop and run them pre-commit in the p4lang/open-p4studio repository. This seems like much less moving of source code around between repos, but it also means that p4lang/p4c pre-commit CI runs won't run the Tofino assembler.

I don't have a strong opinion of one of these choices over the other. The amount of work required for my option #1 definitely sounds significantly larger. That is a big disadvantage, unless you find (or are yourself) a volunteer to make it all work.

@fruffy
Copy link
Contributor

fruffy commented Jan 28, 2025

There are currently no tests for the Tofino compiler, true.
2) is actually not too much work but someone has to do it. The best way is to create a branch on p4lang/p4c, open a PR there to test it, then create a branch here that updates the p4c submodule to point to the new p4lang/p4c branch.

@ChrisDodd
Copy link

ChrisDodd commented Jan 28, 2025

@jafingerhut -- we have two options here:

  1. Leave p4lang/p4c code as-is, but add the code in p4studio that will transform the code during the installation
  2. Modify the code in p4lang/p4c

I think the second option is better -- and I think it should be set up to run the assembler by default if it is installed and not if it is not installed -- the python script should look to see if a bfas binary is there and run it if it is, but not crash or error out (perhaps a warning message) if it is not.

@fruffy fruffy added the bug Something isn't working label Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants