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 p4lang/p4-spec#1308

[vlstill]

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).

Sounds reasonable. Is there a risk that the annotation will currently inhibit some optimization? That would be very unfortunate.

However, if we start merging blocks where the annotations don't match exactly, could we end up with both @likely and @unlikely on a single blocks? What should be done then? Imagine code like this:

if (some_runtime_condition) @likely {
    if (some_condition_compiler_proves_true) @unlikely {
        code;
    }
}

obviously, this example is a bit pathological as the code claims the branch is unlikely, but the compiler proves it can be taken always.

[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?

[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.