[PATCH] D132408: [SimplifyCFG] accumulate bonus insts cost

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl marked 4 inline comments as done.
yaxunl added a comment.

In D132408#3756337 <https://reviews.llvm.org/D132408#3756337>, @nikic wrote:

> Do you have any numbers for the impact of this change? I suspect (but haven't checked) that it may be quite significant, because the current tiny bonus inst threshold (1) is tuned for the current implementation, and this patch will result in much less common dest folding than would be desirable.

For a synthetic benchmark geared towards loops with small workloads, we saw around 16% performance gain due to avoiding always executing 31 extra load instructions by not folding 31 basic blocks coming from an unrolled loop.

For typical HIP programs, I did not see significant performance differences before and after this change. This is because the original bonus instruction threshold 2 is very small. It only allows folding small basic blocks which only contain one instruction other than the cmp/branch instructions. In practical applications, such basic blocks are rare and usually are not performance bottlenecks. Only in special situations (e.g. a fully unrolled loop of large loop counts ends up with a large number of foldable basic blocks) the performance penalty becomes significant.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:211
+    NumBonusInsts.insert({BB, 0});
+    Loc = NumBonusInsts.find(BB);
+  }
----------------
nikic wrote:
> insert returns the iterator, no need to call find again.
> insert returns the iterator, no need to call find again.

will do


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:219
+    return 0;
+  return Loc->second;
+}
----------------
nikic wrote:
> You can use lookup() here, which will return 0 if not found.
> You can use lookup() here, which will return 0 if not found.

will do


================
Comment at: llvm/test/Transforms/SimplifyCFG/branch-fold-unrolled-loop.ll:3
+
+target triple = "amdgcn-amd-amdhsa"
+
----------------
fhahn wrote:
> Does the test depend on the target triple? If not , it should be removed. If it does it needs `REQUIRES:` to only run it when the AMDGPU backend is built.
> 
> Also, can you add the test in a separate patch and only include the changes caused by the patch in the diff so it is easier to see what's going on?
> 
> It also seems like the test coverage is quite limited. Are there some edge cases that should be covered by extra tests?
> Does the test depend on the target triple? If not , it should be removed. If it does it needs `REQUIRES:` to only run it when the AMDGPU backend is built.
> 

The test depends on the target triple. Will add REQUIRES.

> Also, can you add the test in a separate patch and only include the changes caused by the patch in the diff so it is easier to see what's going on?
> 

Will add this test in a separate patch and include the change only.

The difference is that: before this patch 3 BB's are folded into the first BB, accumulating 3 bonus insts; after this patch, only 2 BB's are folded, each accumulating 1 bonus inst.

> It also seems like the test coverage is quite limited. Are there some edge cases that should be covered by extra tests?

Will add more tests for edge cases.



================
Comment at: llvm/test/Transforms/SimplifyCFG/branch-fold-unrolled-loop.ll:8
+; CHECK-LABEL: define {{.*}}@test(
+; CHECK-LABEL: entry:
+; CHECK:       %arrayidx = getelementptr inbounds %struct.S, %struct.S* %this, i64 0, i32 0, i64 0
----------------
tra wrote:
> It's not quite clear what's the purpose of this test. I.e. what are the important pieces of the checks below?
> 
> I'd add a comment describing expected transformation.
> It's not quite clear what's the purpose of this test. I.e. what are the important pieces of the checks below?
> 
> I'd add a comment describing expected transformation.

will add a comment about expected transformations.


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

https://reviews.llvm.org/D132408