[PATCH] D132910: [SimplifyCFG] add a test for branch folding multiple BB

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!



================
Comment at: llvm/test/Transforms/SimplifyCFG/branch-fold-multiple.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt %s -S -o - -simplifycfg | FileCheck %s
+
----------------
please use the newpm syntax: `-passes=simplifycfg`


================
Comment at: llvm/test/Transforms/SimplifyCFG/branch-fold-multiple.ll:10
+
+define zeroext i1 @test1(i32 %0, i32 %1, i32 %2, i32 %3) unnamed_addr align 2 {
+; CHECK-LABEL: @test1(
----------------
nit: drop unneeded `zero ext` `unnamed_addr align 2`


================
Comment at: llvm/test/Transforms/SimplifyCFG/branch-fold-multiple.ll:56
+; Check the second, third, and forth basic blocks are folded into the first
+; basi block since each has no bonus instruction.
+
----------------
nit: `basi -> basic`


================
Comment at: llvm/test/Transforms/SimplifyCFG/branch-fold-multiple.ll:97
+; since it has three bonus instructions, which exceeds the default bonus
+; instruction threshold 1. The third and fourth basic blocks are folded
+; into the second basic block since they do not have bonus instruction.
----------------
nit: of 1.


================
Comment at: llvm/test/Transforms/SimplifyCFG/branch-fold-multiple.ll:5
+
+target triple = "amdgcn-amd-amdhsa"
+
----------------
yaxunl wrote:
> yaxunl wrote:
> > fhahn wrote:
> > > Does this need to be target specific? We should definitely have at least some target-independent tests here.
> > The cost of bonus instructions depends on the target triple and processor (https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L3676). Certain processors have high costs than others, causing the branch not to be folded.
> > 
> > For example, if I replace the target triple of this lit test with `x86_64` and remove the target processor. This lit test still passes since `x86_64` behaves similarly as amdgcn gfx906. However, if I replace the target triple with `x86`, this lit test will fail since the branches are never folded even without my patch.
> > 
> > How about I make the IR in this lit test target independent, and use `-mtriple` to test a few triples which are relevant to the test?
> > The cost of bonus instructions depends on the target triple and processor (https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L3676). Certain processors have high costs than others, causing the branch not to be folded.
> > 
> > For example, if I replace the target triple of this lit test with `x86_64` and remove the target processor. This lit test still passes since `x86_64` behaves similarly as amdgcn gfx906. However, if I replace the target triple with `x86`, this lit test will fail since the branches are never folded even without my patch.
> > 
> > How about I make the IR in this lit test target independent, and use `-mtriple` to test a few triples which are relevant to the test?
> 
> The drawback of the above approach is that it requires multiple registered targets, which can make it less tested on bots.
> 
> Another way is to use the existing option for bonus insts threshold. Increase it so that the lit test behaves the same on all targets. Then this lit test will be target independent.
> Certain processors have high costs than others, causing the branch not to be folded.

Right, 


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

https://reviews.llvm.org/D132910