[PATCH] D158668: RFC: Add getLikelyBranchWeight helper function

[Wenlei He via Phabricator via cfe-commits]

wenlei added a comment.

In D158668#4611988 , @MatzeB wrote:

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

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158668/new/

https://reviews.llvm.org/D158668

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158668: RFC: Add getLikelyBranchWeight helper function

[Matthias Braun via Phabricator via cfe-commits]

MatzeB added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158668/new/

https://reviews.llvm.org/D158668

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158668: RFC: Add getLikelyBranchWeight helper function

[Matthias Braun via Phabricator via cfe-commits]

MatzeB added a comment.

> 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 
`__attribute__((expected))` corresponding to a 99:1 ratio.

Anyway we probably need a separate/patch discussion to not derail this diff too 
much...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158668/new/

https://reviews.llvm.org/D158668

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158668: RFC: Add getLikelyBranchWeight helper function

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

In D158668#4611842 , @MatzeB wrote:

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

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.

In D158668#4611845 , @MatzeB wrote:

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

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158668/new/

https://reviews.llvm.org/D158668

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits