Skip to content

[Tofino] Replace bf-asm BUG macros with top-level BUG macros. Remove GNU extensions flags. #5188

Merged
fruffy merged 8 commits intomainfrom
fruffy/bf_asm_fixes
Apr 2, 2025
Merged

[Tofino] Replace bf-asm BUG macros with top-level BUG macros. Remove GNU extensions flags. #5188
fruffy merged 8 commits intomainfrom
fruffy/bf_asm_fixes

Conversation

@fruffy
Copy link
Collaborator

@fruffy fruffy commented Mar 20, 2025

  • Remove GNU extensions for bfasm. Seems to produce compilation problems on some recent systems.
  • Instead, use the C++20 feature __VA_OPT__ which only emits is argument when there are more variadic args. This seems to compile on all systems, but I need to add some more checks.
  • Replace the BUG macros of bfasm with the top-level BUG macros. Also make sure every BUG call actually has a diagnostic. Tried to produce sensible messages for all the calls, but might have slipped in a few places.

TODO: Recheck error messages and consider emitting a output that is different from Compiler Bug?

@fruffy fruffy added the tofino Topics related to the Tofino switch and back end. label Mar 20, 2025
@fruffy fruffy force-pushed the fruffy/bf_asm_fixes branch 7 times, most recently from 513f8b5 to 1a40489 Compare March 25, 2025 13:10
@fruffy fruffy marked this pull request as ready for review March 26, 2025 06:06
@fruffy fruffy requested a review from ChrisDodd March 26, 2025 06:06
@fruffy
Copy link
Collaborator Author

fruffy commented Mar 26, 2025

@ChrisDodd I couldn't find a way to rewrite the EXPAND_COMMA macros safely, so I resorted to VA_OPT. Also @jafingerhut had trouble compiling the BUG macros so I replaced them and added diagnostics. Let me know if you other suggestions or see something incorrect.

@fruffy fruffy changed the title Fix a couple bf-asm compilation problems. [Tofino]: Replace bf-asm BUG macros with top-level BUG macros. Remove GNU extensions flags. Mar 26, 2025
@fruffy fruffy changed the title [Tofino]: Replace bf-asm BUG macros with top-level BUG macros. Remove GNU extensions flags. [Tofino] Replace bf-asm BUG macros with top-level BUG macros. Remove GNU extensions flags. Mar 26, 2025
@fruffy fruffy force-pushed the fruffy/bf_asm_fixes branch from 1a40489 to 6a38dd5 Compare March 31, 2025 08:31
@fruffy
Copy link
Collaborator Author

fruffy commented Mar 31, 2025

@ChrisDodd This code touches a lot of files, but most of the changes are just there to add diagnostics to BUG/BUG_CHECK. Could you give this a review? This PR is required to finally merge bf-asm into the studio here: p4lang/open-p4studio#70

I am using VA_OPT although we are not quite C++20 yet. Thankfully, this works on all the systems we test on. It's the only way I have been able to get rid of the gnu-extensions and variadic arguments, which cause compilation problems for various compiler distributions.

@ChrisDodd
Copy link
Contributor

ChrisDodd commented Mar 31, 2025

The compiler BUG macros throw an exception rather than printing a message and then exiting. Do we want a top-level exception handler in main (in bfas.cpp) to catch these exceptions and print the message?

*/
#define EXPAND(...) __VA_ARGS__
#define EXPAND_COMMA(...) __VA_OPT__(, )##__VA_ARGS__ // NOLINT
#define EXPAND_COMMA_CLOSE(...) __VA_OPT__(, ) ##__VA_ARGS__ __VA_OPT__()) // NOLINT
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the ## be here? In addition, the ) needs to be macro-ized to make paren handling correct. so should be:

#define CLOSE_PAREN   )
#define EXPAND_COMMA_CLOSE(...) __VA_OPT__(, ) __VA_ARGS__ __VA_OPT__(CLOSE_PAREN)

Copy link
Contributor

@ChrisDodd ChrisDodd Mar 31, 2025

Choose a reason for hiding this comment

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

Or maybe it should always have the ), so the second __VA_OPT__ is not needed. Should be

#define EXPAND_COMMA_CLOSE(...) __VA_OPT__(, ) __VA_ARGS__  )

-- that seems to be what the original code was doing

Copy link
Contributor

Choose a reason for hiding this comment

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

It is probably ok like this -- __VA_OPT__() is a noop no matter how you look at it, and the spurious ## is probably just ignored, though a strict reading of the spec suggests that having it here is undefined.

fruffy added 6 commits April 2, 2025 09:13
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
@fruffy fruffy force-pushed the fruffy/bf_asm_fixes branch from 3cc40ad to 2da7d00 Compare April 2, 2025 07:13
fruffy added 2 commits April 2, 2025 09:38
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
@fruffy fruffy force-pushed the fruffy/bf_asm_fixes branch from 2da7d00 to 103d3c4 Compare April 2, 2025 07:39
@fruffy fruffy added this pull request to the merge queue Apr 2, 2025
Merged via the queue into main with commit 17ade03 Apr 2, 2025
20 checks passed
@fruffy fruffy deleted the fruffy/bf_asm_fixes branch April 2, 2025 10:12
@kfcripps
Copy link
Contributor

@fruffy @ChrisDodd After this PR, I am getting a bunch of errors like this each time I try to locally run ninja clang-format or ninja clang-format-fix-errors:

...
Error reading /local/kfcripps/repos/p4c/.clang-format: Invalid argument
Formatting /local/kfcripps/repos/p4c/tools/ir-generator/methods.cpp
YAML:31:3: error: unknown key 'WhitespaceSensitiveMacros'
  - __VA_OPT__
  ^
Error reading /local/kfcripps/repos/p4c/.clang-format: Invalid argument
Formatting /local/kfcripps/repos/p4c/tools/ir-generator/type.cpp
YAML:31:3: error: unknown key 'WhitespaceSensitiveMacros'
  - __VA_OPT__
  ^
Error reading /local/kfcripps/repos/p4c/.clang-format: Invalid argument
Formatting /local/kfcripps/repos/p4c/tools/ir-generator/type.h
YAML:31:3: error: unknown key 'WhitespaceSensitiveMacros'
  - __VA_OPT__
  ^
Error reading /local/kfcripps/repos/p4c/.clang-format: Invalid argument
clang-format failed. Run make clang-format-fix-errors to fix the complaints.
ninja: build stopped: subcommand failed.

Do you have any idea how to resolve this?

@fruffy
Copy link
Collaborator Author

fruffy commented May 12, 2025

Which version of clang-format are you using? Seems to be a problem with the config file.
clang-format 18.1.0 is the version the compiler uses.

@kfcripps
Copy link
Contributor

@fruffy I am using clang-format version 18.1.8

@fruffy
Copy link
Collaborator Author

fruffy commented May 13, 2025

Let me see whether it can be reproduced with that version..

@fruffy
Copy link
Collaborator Author

fruffy commented May 14, 2025

I can not reproduce it locally and with CI (#5272). Maybe the CMake script picks up a dated clang-format version?

@kfcripps
Copy link
Contributor

@fruffy After doing a clean build, clang-format commands seem to work again. Thanks for checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tofino Topics related to the Tofino switch and back end.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants