[PATCH] D131287: Fix branch weight in FoldCondBranchOnValueKnownInPredecessor pass in SimplifyCFG

Mon Aug 22 10:42:48 PDT 2022

paulkirth added a comment.

In D131287#3737801 <https://reviews.llvm.org/D131287#3737801>, @LukeZhuang wrote:

> (1) I agree that using blockfrequency(like the function that nikic suggested) is the best way to solve this issue. I was trying to use BlockFrequencyInfo at the beginning but I found this information is not maintained and no otherwhere else use this in SimplifyCFG. May I ask what do we usually do for this situation? Is that adding this in SimplifyCFG?

I'm not completely sure I follow you here. Are you asking if it's OK to use `BlockFrequencyInfo` + `BlockProbabilityInfo` in SimplifyCFG? I don't see why not if that is the correct thing to do and avoids complexity. Those are analysis passes, so should be safe to take a dependency on. SimplifyCFG is going to invalidate them though, so it's going to introduce overhead. IMO correctness is more important though, so I wouldn't worry about running a (usually quick) set of analysis passes.

The other thing to consider is if JumpThreading should have that functionality extracted to some common place so SimplifyCFG can share the implementation. That would be my preference, but there may be reasons to avoid that.

> (2) Jumpthreading seems is doing the same thing as FoldCondBranchOnValueKnownInPredecessor in SimplifyCFG, may I ask do you know(or someone else know) why we need both of them? And jumpthreading seems is not in -O1 optimization. I guess if we can use jumpthreading, this issue would not exist at the beginning.

I'm not an authority on this, so please take my comments here with a grain of salt.

Jump threading is a pretty well known type of optimization. The change in the CFG you're patching up is an example of jump threading: find a block that always jumps to another and bypass the intermediate block. It's simple/safe enough that the maintainers of SimplifyCFG felt that it belonged here. This can be a pretty profitable optimization, so I'm not surprised that's the case.

It's my understanding that the JumpThreadingPass does more sophisticated things than SimplifyCFG, and you may not always want to do that. In some cases, those optimizations can make loops irreducible, so LLVM has historically been conservative w/ it's jump threading implementation. I also believe that's why it only runs a higher optimization levels, but someone with more familiarity with this part of LLVM can probably provide a more complete answer than I can.

> Any way, thank you for the comments!

Happy to help, but, again, I'm not sure I'm the most qualified reviewer for this...

BTW,  did you look into the rest of SimplifyCFG to see if there are more systemic issues w/ how branch weights are propagated? If there are, a more general solution/refactor may be warranted, and I would advise you to bring it up on discourse to get more interest/feedback.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131287