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?

[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

[vlstill]

What should be the intended meaning of these cases:

apply {
    @likely {
        // code (1)
    }
    @unlikely {
        // (2)
    }
    if (foo) {
        // ...
    } else @likely {
        // (3)
    }
}

They are permitted by the grammar, should there be warning for (2), maybe even for (1) as it is is suspicious? Is (3) intended to be equivalent to having the opposite annotation on the "then" branch. Maybe there should be just a general warning pass that warns on any unconditional @likely/@unlikely and maybe even simplifies case (3). Plus there probably should be warning for something like if (foo) @likely @unlikely which is also entirely permissible in the grammar.

---

I was thinking if there is a better way than attaching this just to block statements. But probably not, it does not seem right to have a special attachment point for annotations in if/switch just for this case. This does not work for non-block-statement if though, because statements generally aren't annotated. Again, this is probably OK.

We might want to extend selectCase to something like this though:

selectCase
    : keysetExpression ":" optAnnotations name ";"
    ;

---

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

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

[asl]

See comments

[asl]

Won't it be too verbose? I can easily see that unlikely case could appear after some e.g. generic routine would be inlined and then condition will be constfolded on e.g. on error path? Something like this:

function generic() {
  if (not error)  @likely {
   dosomething();
  } else {
    dootherthing();
  }
}

function handle_error() {
  generic();
}

As a comparison, C/C++ compilers do not produce diagnostics while constfolding __builtin_expect and friends.

[ChrisDodd]

Trying to find a happy medium between too verbose and not verbose enough. @vlstill requested this warning, and it is just a warning. Perhaps leave it and see if anyone complains?

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