Add @likely/@unlikely annotations for blocks#4979
Conversation
d86089f to
d482036
Compare
46257c9 to
1847985
Compare
vlstill
left a comment
There was a problem hiding this comment.
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/@unlikelyon 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.
1847985 to
e5e00c1
Compare
b116b77 to
ecb291c
Compare
|
So it seems that something like is quite plausible, and if the compiler can prove it is always true and remove the |
05e10e9 to
da01db2
Compare
da01db2 to
bbc2c76
Compare
f1652aa to
35b4a33
Compare
35b4a33 to
665e0ad
Compare
962d58e to
da05bef
Compare
|
@fruffy @jafingerhut -- can I get approval for this PR? |
|
@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. |
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 |
|
I commented out the warning for @unlikely on always taken blocks. Unfortunately looks like all the CI is broken currently |
6fd64dd to
91b031d
Compare
- fix a few places annotations on blocks are accidentally lost - allow merging blocks with same annotation Signed-off-by: Chris Dodd <cdodd@nvidia.com>
Signed-off-by: Chris Dodd <cdodd@nvidia.com>
- warn if @unlikely is always taken. Signed-off-by: Chris Dodd <cdodd@nvidia.com>
Signed-off-by: Chris Dodd <cdodd@nvidia.com>
|
@ChrisDodd seems |
Yes, I only disabled the warning for an always taken 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 |
Signed-off-by: Chris Dodd <cdodd@nvidia.com>
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/@unlikelyon 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