Add @likely/@unlikely annotations for blocks

[ChrisDodd]

These will generally be used by backends and simply passed through the frontend/midend, so not much is needed here.

Perhaps some frontend passes (in particular, DoSimplifyControlFlow in frontends/p4/simplify.cpp) could be extended to understand these -- currently, any annotations on a block will inhibit combining the blocks, whereas @likely/@unlikely on a block should not inhibit combining it with a block with no annotation (the annotation should be on the resulting combined block).

See discussion in https://github.com/p4lang/p4-spec/issues/1308

[ChrisDodd]

So it seems that something like

if (condition_that_is_always_true) @likely {  ...some code... }

is quite plausible, and if the compiler can prove it is always true and remove the if, it should probably also remove the @likely. For the more pathological case of having an @unlikely here, it should probably also remove it, as it just seems wrong. Maybe a warning about that?

[ChrisDodd]

So I've added warnings for always taken/@unlikely and never taken/@likely branches and a testcase. Overall, I think this is ready to go -- do we need more discussion in the LDWG next week?

[jafingerhut]

@asl @fruffy @vlstill (and anyone else interested, of course) -- We discussed this PR, and the corresponding issue considering adding these new annotations to the language spec, at the 2025-Jan-13 language design work group meeting. Chris is hoping that these changes can be approved and merged before he starts work on a PR for the language spec describing these annotations.

[ChrisDodd]

@asl @fruffy @vlstill @jafingerhut -- is it possible to get a review/approval for this?

[asl]

@asl @fruffy @vlstill @jafingerhut -- is it possible to get a review/approval for this?

Thanks! I will review soon

[ChrisDodd]

Summary: This looks reasonable to me. I think the if (true) @unlikely warning should be added in this PR, the rest can be probably done later (including the select expression for its similarity with switch).

I added that (as well as if (false) @likely) warning. However, there's currently no message for @likely/@unlikely on a block that is not part of an if or case. A backend can take such annotations into account if it wants to put a meaning on them.

[ChrisDodd]

I think this PR is ready to go, except for discussion on what should and should not be a warning. @asl, @vlstill what do you think?

[ChrisDodd]

@fruffy @jafingerhut -- can I get approval for this PR?

[jafingerhut]

@ChrisDodd Given my familiarity with only a tiny subset of the C++ implementation code for p4c, unfortunately I don't think I'm qualified to review that, and that is pretty critical here.

[asl]

I think this PR is ready to go, except for discussion on what should and should not be a warning. @asl, @vlstill what do you think?

Yeah. @vlstill what do you think for the warning? I doubt that it should be there and e.g. existing practice with C/C++ compilers with __builtin_expect shows that we might easily inline through unexpected branch (e.g. inside error path handling) and therefore things will be too verbose.

[ChrisDodd]

I commented out the warning for @unlikely on always taken blocks. Unfortunately looks like all the CI is broken currently

[asl]

@ChrisDodd seems annotation-likely.p4-stderr requires an update. And it seems some warnings are still produced?

[ChrisDodd]

@ChrisDodd seems annotation-likely.p4-stderr requires an update. And it seems some warnings are still produced?

Yes, I only disabled the warning for an always taken @unlikely branch. It still warns for a never taken @likely branch.

Perhaps both these warnings should be in their own warning class that defaults to off, but can be turned on with an appropriate -Wwarn option

[asl]

Perhaps both these warnings should be in their own warning class that defaults to off, but can be turned on with an appropriate -Wwarn option

Looks reasonable, yes.

[ChrisDodd]

Perhaps both these warnings should be in their own warning class that defaults to off, but can be turned on with an appropriate -Wwarn option

Looks reasonable, yes.

So I actually did this -- all these warnings now default to off, but can be turned on with --Wwarn=branch. Its a little bit messy -- I had to add an initReporter method to ErrorCatalog to set the default initial state of error reporting.