Add the Barefoot Tofino Assembler to the Tofino back end. #5121
Conversation
b9b092b to
0abf784
Compare
a28a715 to
a2c1c66
Compare
There was a problem hiding this comment.
@vlstill Feel free to give an actual detailed review (unlike this one, which is based on CI tests passing, the fact that these changes should affect ONLY the Tofino back end, and wanting to get the other parts of this working in the open-p4studio repo so we can leave it in a mostly-working state for others). However, as far as I can see right now, Fabian is the only one with the knowledge and interest to make these changes, and I'm the only other one interested in seeing that work move forward.
I have probably over-shared my qualifications on this approval :-)
|
Well, I guess as I originally wrote much of this code, I'm qualified to comment on it, if not review it. The stuff in bf-asm/test is an old broken (non-functional) test suite that is based on running the even older python-based P4_14 compiler, and should probably just be deleted (nothing in it is used, and I don't think any of it is useful) It looks like it does build fine (with just It does have additional dependencies to build (I noticed at least pyyaml -- needed |
Okay, I will remove that code.
Currently, compilation should be covered by the P4Testgen tests which runs the compiler with the following arguments: https://github.com/p4lang/p4c/blob/main/backends/p4tools/modules/testgen/targets/tofino/test/TestTemplate.cmake#L94 But it is not running packet-tests yet because the model isn't integrated. Most of them seem related to particular macro usage in the tables file. The expansion causes additional |
Most of the warning seem to come from base class virtual methods that have unused arguments -- these arguments are named for documentation/clarity (basically noting what the derived classes will do with them) even though they are not used in the base class method. Fixing the warnings by removing the arguments will make the code less self-documenting and harder to read/understand. I guess the arg names should be replaced by comments to both keep the info and silence the warnings. |
097b730 to
7229ed3
Compare
Slowly working my way through the warnings. There are a lot of warnings I can not fix because they are generated using |
806c7a8 to
d200785
Compare
4f23857 to
fa1091b
Compare
|
@ChrisDodd If you have some time, could you give the last couple commits a review? I tried to fix some warnings but some of the changes I am not 100% sure about. |
fa1091b to
e05cc32
Compare
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
e05cc32 to
692528b
Compare
Signed-off-by: fruffy <fruffy@nyu.edu>
backends/tofino/bf-asm/salu_inst.cpp
Outdated
| @@ -551,7 +551,7 @@ Instruction *AluOP::pass1(Table *tbl_, Table::Actions::Action *act) { | |||
| if (k1 && (k1->value < Target::STATEFUL_ALU_CONST_MIN() || | |||
| k1->value > Target::STATEFUL_ALU_CONST_MAX())) { | |||
| if (k1->value >= (INT64_C(1) << tbl->alu_size()) || | |||
| k1->value < (INT64_C(~0u) << (tbl->alu_size() - 1))) { | |||
| k1->value < (~UINT64_C(0) << (tbl->alu_size() - 1))) { | |||
There was a problem hiding this comment.
To do a signed comparison, you need to cast the value to a signed type (int64_t). I would expect this to give a signed vs unsigned comparison warning (and will do an unsigned comparison, which is wrong.)
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
There was a problem hiding this comment.
Why is this using FetchContent if the dir is in repo (submodule?).
There was a problem hiding this comment.
It looks like it was contributed this way. I can change the installation direction to the cmake build folder instead.
Co-authored-by: Vladimír Štill <git@vstill.eu> Signed-off-by: Fabian Ruffy <5960321+fruffy@users.noreply.github.com>
We do have around 50 tests for TNA and T2NA as part of P4Testgen's workflow, which also invokes the compiler and assembler. I can also add the P4-16 BMV2 tests? |
Ok, that is probably enough. As long as there is something that is executing at least some of the code. It still would be nice to do some gcov (or similar) analysis of the compiler running the testsuite to see what code is (not) excercised anywhere in the testsuite. |
|
Merging this one. The other issues I can try to resolve in subsequent PRs. |
The Tofino compiler is not really useful without the assembler. Hence we add it directly. Now the assembler is always active when we use
bf-p4c.The problem is that the assembler code is a little rough and does not pass many of the linting checks P4C has. This required a couple fixes and workarounds. I commented the remaining TODOs in the CMake files. Ideally, we gradually fix all the warnings that are thrown over time.