[PATCH] D158668: RFC: Add getLikelyBranchWeight helper function

Wed Aug 23 16:29:50 PDT 2023

paulkirth added a comment.

In D158668#4611842 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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