- Add getLikelyBranchWeight() function returning the value of the -likely-branch-weight option defaulting to 2000.
- Remove -unlikely-branch-weight option and just use 1.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
My initial reaction to this was that we should keep the --unlikely-branch-weights flag available, even if we decide to move forward with the other changes. but given that you haven't had to update many tests, I think its probably fine to go this way. Probably just an FYI on discourse is sufficient to notify potential downstream consumers.
As for exposing the Likely/unlikely weights ... I'm a bit conflicted. On one hand, I would love a simple way to query the current weights for MisExpect diagnostics, and some of the other things we've been prototyping like suggesting places where annotations would be profitable. On the other hand, I dislike exposing some internal detail of a pass.
That said, it seems like these have more value when exposed, so I think it woudl be fine, as long as we can prevent frontends from using the APIs, like you mentioned in https://reviews.llvm.org/D158642#4611025.
I'm not sure how to separate the APIs the way we want, though. Most of the APIs in "ProfDataUtils.h" are desirable to be public, which means include is a good place for them, but getLikelyBranchWeight may not fit well in that regard... Maybe we need an internal header in lib? or perhaps I'm thinking bout this too much...
My initial reaction to this was that we should keep the --unlikely-branch-weights flag available
I don't feel strongly about it and can put it back. But can you give some reasoning? I only see this flag having a real use to express small ratios like 3:2 which doesn't really seem helpful to express "likely"/"unlikely"...
On the other hand, I dislike exposing some internal detail of a pass.
I think the question to ask is whether a concept like "likely branch" or "unlikely branch" shouldn't be more universal and not be a pass internal detail? Otherwise it feels like my patches and other places would need to insert new llvm.expect usages into the IR and add more instances of LowerExpect pass into the pipeline to not risk pass ordering problems for the small gain of having a more abstract representation...
And as another strawman / discussion-starter I put up D158680 where I use !{"branch_weights", i32 1, i32 0} to represent likely branches and the actual "LikelyWeight" mostly becomes an internal implementation detail of the BranchProbabilityInfo class... This seemed like a neat way to get an abstract "likely", "unlikely" notation, but not sure of the effects if we no longer would have "truly zero" weights because they would be interpreted differently now...
I think the rest of the comment agrees w/ you that it probably isn't that useful :)
My concern was testing, but given that you've barley had to update anything, I'd say that early feeling is just wrong, since we aren't using it except in a handful of cases. As such I think it can go away w/ only a short FYI about a planned change.
Again, I think the comments directly following agree w/ your position:
That said, it seems like these have more value when exposed, so I think it would be fine, as long as we can prevent frontends from using the APIs, like you mentioned in https://reviews.llvm.org/D158642#4611025.
I think he fact that we're discussing this means it isn't functionally internal anymore, so that's maybe reason enough to do this.
So to clarify, I'm broadly in favor of this direction. I'd also posit, that maybe since we're changing this we should reevaluate the numbers we use as defaults.
I'd also posit, that maybe since we're changing this we should reevaluate the numbers we use as defaults.
Heh, same here. Internally we have a handful of functions that end up using [[likely]] loop conditions in a triple-nested loops leading to the estimated block frequencies going through the roof and unjustly dominating everything else in the program... Though admittedly I am not decided yet whether I want to blame the 2000:1 ratio or rather the programmers for using the annotation too freely.
There is also a disconnect with the GCC documentation saying __builtin_expect corresponding to a 90% probability ( https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Other-Builtins.html )
Anyway we probably need a separate/patch discussion to not derail this diff too much...
I have a feeling @mtrofin would prefer pass-specific weights rather than a unified notion of "likely"/"unlikely"... So with the latest round of patches it's probably best to abandon this for now.
Commented on that patch, I'm not convinced that canonicalize {X,0} -> {1,0} is a good idea mostly because: 1) we don't attempt to canonicalize {X*Z,Y*Z} -> {X,Y}; 2) there are places where the absolute values matters; 3) canonicalize branch_weights as a side effect of BPI seems not right.
Remove -unlikely-branch-weight option and just use 1.
Seems fine, I don't have a strong opinion.
Add getLikelyBranchWeight() function returning the value of the -likely-branch-weight option defaulting to 2000.
This makes sense to me.
prefer pass-specific weights rather than a unified notion of "likely"/"unlikely"... So with the latest round of patches it's probably best to abandon this for now.
I don't see them as mutually exclusive. In fact when I look at your other patches, I feel that we need a default likely/unlikely for cases where we just don't have a good answer, hence fall back to default. So the default likely/unlikely probability is a layer below pass-specific setting.