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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/test/Transforms/SimplifyCFG/fold-cond-bi-fix-weight.ll:12-13
+;; Actual probability of hitting foo is 1/10(10%)
+;; Before fix, the probability of hitting foo is 11/20(55%)
+;; After fix, the probability of hitting foo is 29/200(14.5%)
+define void @branchinst_noweight(i8 %a0, i8 %a1, i8 %a2) {
----------------
paulkirth wrote:
> I don't know if these comments(here and elsewhere) make sense outside of this patch, since 'Before' and 'After' are only relevant in the context of your change...
> 
> I think if you want to more thoroughly explain the purpose of the test, you'll need to provide context. A summary of the incorrect behavior and what a regression looks like is probably enough. You can also ref this patch, and anyone coming along later can easily understand why this test exists, and what it's checking.
> 
> WDYT?
The comment can be enhanced for reader to follow. Can you add an ascii art of the CFG after simplifyCFG 


================
Comment at: llvm/test/Transforms/SimplifyCFG/fold-cond-bi-fix-weight.ll:90
+;; Before fix, the probability of hitting foo is 4/100(4%)
+;; After fix, the probability of hitting foo is 904/1000(90.4%)
+define void @switch_test(i32 %a) {
----------------
Similarly add a cfg description post transformation.


================
Comment at: llvm/test/Transforms/SimplifyCFG/fold-cond-bi-fix-weight.ll:132
+;; After calculation, the probability of hitting foo is roughly the same.
+define void @overflow_test(i32 %a) {
+; CHECK-LABEL: @overflow_test(
----------------
Can you split out the overflow fix in a different patch?


================
Comment at: llvm/test/Transforms/SimplifyCFG/fold-cond-bi-fix-weight.ll:175
+;; after fix,  they are: func1: 100%, func2: 1.4%, func3: 1.5%
+define void @more_complex_test(i32 %a) {
+; CHECK-LABEL: @more_complex_test(
----------------
Add post simplfycfg CFG 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131287