Support inferring which architecture file to include from --target command line option. #1177
Conversation
| std::tie(device, arch, vendor) = options.parseTarget(); | ||
|
|
||
| options.preprocessor_options += " -D__TARGET_EBPF__"; | ||
| if (arch == "ss") { |
There was a problem hiding this comment.
This is wrong, you can never compile ebpf programs for v1model or PSA targets.
There was a problem hiding this comment.
Is it impossible to support psa on the ebpf target/backend? (psa and v1model are architectures, not targets -- ideally you could compile any architecture on any target, or at least get a clear message that that architecture/target combination is not supported).
There was a problem hiding this comment.
Take a look at p4include/ebpf_model.p4 and it will be obvious that it is very different from v1model.p4. For example, the ebpf back-end only supports packet filters. It does not have output ports or output packets, just a boolean. The set of externs supported is very different. Table properties are different, for example, there are no ternary key matches.
| #endif | ||
| #endif | ||
|
|
||
| #ifdef __TARGET_EBPF__ |
There was a problem hiding this comment.
This does not work: you cannot list all possible back-end here, because you don't know what they are. Where is the Tofino back-end, or the p4c-xdp back-ends, which are loaded as extensions? This breaks the modularity of the compiler. So far there no one piece that has to know about all back-ends; when you add a back-end it just appears.
If you really want to do this you should do it as part of the build process, and this file should be automatically generated, in the same way makefiles are.
There was a problem hiding this comment.
Since the includes seem purely based on the __ARCH_ symbols, is it even necessary to check __TARGET_anything?
|
|
||
| #ifdef __TARGET_BMV2__ | ||
| #ifdef __ARCH_V1MODEL__ | ||
| #include "v1model.p4" |
There was a problem hiding this comment.
This should be <v1model.p4>. The angle brackets are important.
| std::string target_str(target.c_str()); | ||
| boost::split(splits, target_str, [](char c){return c == '-';}); | ||
| if (splits.size() != 3) | ||
| BUG("Invalid target %s", target); |
There was a problem hiding this comment.
We have never specified as a requirement that target has to look this way. For example, the ebpf back-end has different requirements for the target string.
There was a problem hiding this comment.
The 3-component target is a feature/requirement of the p4c driver program, so if you want a backend to be callable via the driver, it needs to use the same kind of target. If that is not adequate for some target, we need to figure out what needs to be added to the p4c driver
There was a problem hiding this comment.
I have never seen a specification of what the driver is supposed to do.
If the driver specification influences the design of the compiler we have to have it written down somewhere.
| // the p4c driver sets environment variables for include | ||
| // paths. check the environment and add these to the command | ||
| // line for the preprocessor | ||
| char * driverP4IncludePath = | ||
| isv1() ? getenv("P4C_14_INCLUDE_PATH") : getenv("P4C_16_INCLUDE_PATH"); | ||
| cmd += cstring(" -C -undef -nostdinc") + " " + preprocessor_options | ||
| + (driverP4IncludePath ? " -I" + cstring(driverP4IncludePath) : "") | ||
| + " -I" + (isv1() ? p4_14includePath : p4includePath) + " " + file; | ||
| + " -I" + (isv1() ? p4_14includePath : p4includePath) | ||
| + " " + (isv1() ? "" : arch) + " " + file; |
There was a problem hiding this comment.
I think that this is not a good idea. A compiler should not magically include files based on esoteric and undocumented environment variables. It will be difficult for the user to understand what is going on. The compiler should include exactly the files that the user specifies in the program using #include. Then the compilation process is clear to everyone.
There was a problem hiding this comment.
If this is an issue about using the p4c driver program vs calling the backend executable directly, perhaps it should be the driver program adding the argument to its invocation of the preprocessor? Actually, isn't it the driver that calls the preprocessor anyways and calls the backend executable with some flag to disable the preprocessor? Or do we end up running the preprocessor twice when we use p4c?
There was a problem hiding this comment.
Yes, the driver would be the natural place for this.
There was a problem hiding this comment.
The driver calls the preprocessor, but the preprocessor also is called from the compiler. This is why we need deeper changes in the way we handle these includes.
|
@mbudiu-vmw there's been an entire discussion on the driver design here: #378. The triplets are documented in the front README file: https://github.com/p4lang/p4c/blob/1b5a81340eec04a7a2adf3a5d627fc65454cac5d/docs/README.md#compiler-driver From a usability point of view, this is a move in the right direction. This is what we've been hearing from our customers. It also seems counterintuitive for users to invoke directly the backend for a particular target, especially when that backend is not the only executable that needs to be run. Look at gcc and clang, both are hiding backends, assembler, linker, etc. behind target options. So this is nothing out of ordinary, and it is totally necessary for usability on more complex targets (such as Barefoot's). In the same vein, we should also have the compiler include standard includes based on the target. We have been equating the architecture with libc -- well, the compiler initializes the entire libc library (and all its standard objects) without any programmer intervention. This is why it seems quite unnecessary to ask the programmers to explicitly include core.p4 when there is nothing you can do in p4 without core.p4. Similarly, if you already specified a target-architecture triplet on the command line, you should not need to also specify the architecture file as an include. In most cases you will get a cryptic compiler error because some incompatibility between controls. It seems more natural to require specifying the target and the architecture in one place and drive the rest of the compiler to do the right thing. That said, I think that the current PR is a temporary solution, but one that allows the insulation from user changes. We need a more involved solution in the compiler to handle everything properly, but that will take more time. |
|
Even if gcc handles libc, it does not insert |
Users don't have to include
core.p4orv1model.p4in their program, as long as they specify which target the program should be compiled to, the compiler should be smart enough to include the correct file.