Page MenuHomePhabricator

[PGO] Change ThinLTO test for targets with loop unrolling disabled
ClosedPublic

Authored by sherwin-dc on Sep 3 2021, 6:34 AM.

Details

Summary

I am working on a target in a downstream LLVM repo, and it seems that if a target backend chooses to disable loop unrolling this test would fail. A solution would be to modify the test to search for a different string instead.

The specific test checks for if.true.direct_targ which appears in the output when thinlto is not used (ie samplepgo). The same is true for if.false.orig_indirect.

However, if a target disables loop unrolling in the backend, the test fails as if.true.direct_targ no longer appears, though if.false.orig_indirect still does. This can be seen by using a clang pragma to disable loop unrolling in the unroll() function.

For reference, the following files are the outputs of the last 2 test functions being compiled as the test case does, with and without thinlto, and with and without loop unrolling on the latest x86 clang build. The loop unrolling pragma was used to simulate the loop unrolling being disabled in a backend.

// RUN: %clang_cc1 -O2 -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm -o out.ll
// RUN: %clang_cc1 -O2 -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm -flto=thin -o out.ll

Diff Detail

Event Timeline

sherwin-dc created this revision.Sep 3 2021, 6:34 AM
sherwin-dc requested review of this revision.Sep 3 2021, 6:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2021, 6:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
sherwin-dc retitled this revision from Change ThinLTO test for targets with loop unrolling disabled to [PGO] Change ThinLTO test for targets with loop unrolling disabled.Sep 3 2021, 6:42 AM
sherwin-dc added reviewers: danielcdh, davide.
tejohnson added inline comments.
clang/test/CodeGen/pgo-sample-thinlto-summary.c
41–42

It seems this test is explicitly checking for loop unrolling, so I'm not sure if it will fundamentally work the same if loop unrolling is off. Do you know why the below checking that "loop unroll is invoked by normal compile, but not thinlto compile" is not failing due to your target's change?

I wonder if this test should be converted somehow to an LLVM test.

sherwin-dc added inline comments.Sep 3 2021, 10:38 AM
clang/test/CodeGen/pgo-sample-thinlto-summary.c
41–42

Thats a good question. I guess I was too focused on the failing test below.

In the above file samplepgo_nounroll.ll I had ran this test on just the x86 backend without thinlto and with loop unrolling disabled in clang, but the loop still unrolled and the test passed. (Same story with the custom backend I am working on)

After changing the loop iterations from 2 to 3 then the test was failing which should have been the correct behavior with loop unrolling disabled. Not 100% sure on this but it looks as if a SimplifyCFG pass was unrolling the loop when it was 2 iterations, but not 3. So it looks like this needs to be changed if testing whether the loop-unroll pass was used.

Knowing this, I'm not sure what the next step would be. I dont know if any of the upstream backends had disabled loop unrolling, or how this could be tested in a target-independent way. (I'm quite new to LLVM)

sherwin-dc updated this revision to Diff 371036.Sep 7 2021, 4:26 AM
sherwin-dc edited the summary of this revision. (Show Details)

[PGO] Change tests to look at which passes were run

I have changed the unroll and icp tests to instead look at the passes that were run, which are shown with the -mllvm -opt-bisect-limit=-1 flag. This should allow checking that the Loop Unrolling and Indirect Call Promotion passes are run with samplepgo but not thinlto.

To clarify, the backend that I am working on does not disable the Loop Unrolling pass from running, but uses target hooks within the pass to stop the loop from being unrolled, hence this would work for targets that would choose to disable loop unrolling in this way.

The bottom 2 tests are combined together now because of the way the passes are printed out. Though if there is a better way of arranging the tests please let me know. For reference this is what the output looks like:

sherwin-dc updated this revision to Diff 371040.Sep 7 2021, 4:54 AM

[PGO] Change tests to look at passes run

tejohnson added inline comments.Sep 7 2021, 3:25 PM
clang/test/CodeGen/pgo-sample-thinlto-summary.c
1–4

-opt-bisect-limit seems like a roundabout way to get the pass invocations printed. How about just -mllvm -debug-pass=Structure?

41–42

No need for 2 functions named "unroll" and "icp" anymore, since we are just checking the passes. Why not remove both of these, and just use bar below for checking the passes.

41–42

Nit, consolidate this and the prior sentence.

sherwin-dc marked 2 inline comments as done.Sep 8 2021, 4:39 AM
sherwin-dc added inline comments.
clang/test/CodeGen/pgo-sample-thinlto-summary.c
1–4

I had tried using -mllvm -debug-pass=Structure but could not get anything to print out. It only worked when I removed -cc1 -nostdsysteminc and -emit-llvm which is used when lit runs the tests.

sherwin-dc updated this revision to Diff 371315.Sep 8 2021, 4:41 AM
  • Consolidate test cases into one

@tejohnson would you be able to take another look at this again

tejohnson added inline comments.Sep 14 2021, 10:06 AM
clang/test/CodeGen/pgo-sample-thinlto-summary.c
1–4

I see - this doesn't work with the new PM which is now default. For that you can use -fdebug-pass-manager. For the old PM the -mllvm -debug-pass=Structure does work with -cc1 -emit-llvm. Since the first couple invocations appear meant to check the oldPM, I would add an explicit -fno-experimental-new-pass-manager to their invocations, since that is no longer the default.

  • Modify test to correctly indicate old/new PM
sherwin-dc added inline comments.Sep 15 2021, 6:41 AM
clang/test/CodeGen/pgo-sample-thinlto-summary.c
27

When printing out passes with the old PM 'Unroll loops' is printed out twice with sample PGO and once with thin LTO.

thopre added inline comments.Sep 15 2021, 6:41 AM
clang/test/CodeGen/pgo-sample-thinlto-summary.c
3–4

While we now need to explicitely request the old pass manager, the new pass manager is the default so we don't need to be explicit.

tejohnson added inline comments.Sep 15 2021, 8:14 AM
clang/test/CodeGen/pgo-sample-thinlto-summary.c
3

For the newPM, remove -mllvm -debug-pass=Structure since it isn't doing anything, and you are getting the printing from -fdebug-pass-manager. Also suggest moving the -fdebug-pass-manager into the position where you currently have -mllvm -debug-pass=Structure so the printing options are in the equivalent place for both PM invocations.

27

I looked at the old PM, the first one is the createSimpleLoopUnrollPass that does some full unrolling and peeling of small constant trip count loops. Can you just add a comment above the check for the first Unroll loops here for ThinLTO to note this. It's the second invocation that we delay until the ThinLTO backends.

  • Modify test to correctly indicate old/new PM
sherwin-dc marked 4 inline comments as done.Sep 15 2021, 9:19 AM
tejohnson added inline comments.Sep 15 2021, 9:34 AM
clang/test/CodeGen/pgo-sample-thinlto-summary.c
28

Nit: The second one is skipped by the ThinLTO compile step (it's done in the ThinLTO backends).

  • Modify test to correctly indicate old/new PM
sherwin-dc marked an inline comment as done.Sep 15 2021, 10:41 AM
tejohnson accepted this revision.Sep 15 2021, 10:53 AM

lgtm, thanks!

This revision is now accepted and ready to land.Sep 15 2021, 10:53 AM

I dont have commit access, would someone be able to commit on my behalf? Thank You.

Hello,
We are maintaining a downstream version of the monorepo based on the LLVM main branch. We have not transitioned to the new PM yet. In a recent attempt to merge the latest upstream commits into our monorepo we came across the following test failures after your commit.


FAIL: llvm_regressions :: Clang/CodeGen/pgo-sample-thinlto-summary.c

Script:

: 'RUN: at line 1'; /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/clang -cc1 -internal-isystem /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/lib/clang/14.0.0/include -nostdsysteminc -mllvm -debug-pass=Structure -O2 -fno-experimental-new-pass-manager -fprofile-sample-use=/scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/Inputs/pgo-sample-thinlto-summary.prof /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -emit-llvm -o - 2>&1 | /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/FileCheck /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -check-prefix=SAMPLEPGO-OLDPM
: 'RUN: at line 2'; /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/clang -cc1 -internal-isystem /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/lib/clang/14.0.0/include -nostdsysteminc -mllvm -debug-pass=Structure -O2 -fno-experimental-new-pass-manager -fprofile-sample-use=/scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/Inputs/pgo-sample-thinlto-summary.prof /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -emit-llvm -flto=thin -o - 2>&1 | /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/FileCheck /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -check-prefix=THINLTO-OLDPM
: 'RUN: at line 3'; /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/clang -cc1 -internal-isystem /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/lib/clang/14.0.0/include -nostdsysteminc -fdebug-pass-manager -O2 -fprofile-sample-use=/scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/Inputs/pgo-sample-thinlto-summary.prof /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -emit-llvm -o - 2>&1 | /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/FileCheck /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -check-prefix=SAMPLEPGO

: 'RUN: at line 4'; /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/clang -cc1 -internal-isystem /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/lib/clang/14.0.0/include -nostdsysteminc -fdebug-pass-manager -O2 -fprofile-sample-use=/scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/Inputs/pgo-sample-thinlto-summary.prof /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -emit-llvm -flto=thin -o - 2>&1 | /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/FileCheck /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -check-prefix=THINLTO

Exit Code: 1

Command Output (stderr):

+ : 'RUN: at line 1'
+ /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/clang -cc1 -internal-isystem /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/lib/clang/14.0.0/include -nostdsysteminc -mllvm -debug-pass=Structure -O2 -fno-experimental-new-pass-manager -fprofile-sample-use=/scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/Inputs/pgo-sample-thinlto-summary.prof /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -emit-llvm -o -
+ /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/FileCheck /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -check-prefix=SAMPLEPGO-OLDPM
+ : 'RUN: at line 2'
+ /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/FileCheck /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -check-prefix=THINLTO-OLDPM
+ /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/clang -cc1 -internal-isystem /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/lib/clang/14.0.0/include -nostdsysteminc -mllvm -debug-pass=Structure -O2 -fno-experimental-new-pass-manager -fprofile-sample-use=/scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/Inputs/pgo-sample-thinlto-summary.prof /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -emit-llvm -flto=thin -o -
+ : 'RUN: at line 3'
+ /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/clang -cc1 -internal-isystem /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/lib/clang/14.0.0/include -nostdsysteminc -fdebug-pass-manager -O2 -fprofile-sample-use=/scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/Inputs/pgo-sample-thinlto-summary.prof /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -emit-llvm -o -
+ /scratch/gmiller/tools5/llvm_cgt/arm-llvm/RelWithAsserts/llvm/bin/FileCheck /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c -check-prefix=SAMPLEPGO
/scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c:16:15: error: SAMPLEPGO: expected string not found in input
// SAMPLEPGO: Running pass: PGOIndirectCallPromotion on [module]

^

<stdin>:1:1: note: scanning from here
; ModuleID = '/scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c'
^
<stdin>:4:6: note: possible intended match here
target triple = "arm-ti-none-eabi"

^

Input file: <stdin>
Check file: /scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c

-dump-input=help explains the following input dump.

Input was:
<<<<<<

1: ; ModuleID = '/scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c'

check:16'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found

2: source_filename = "/scratch/gmiller/tools5/llvm_cgt/llvm-project/clang/test/CodeGen/pgo-sample-thinlto-summary.c"

check:16'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

3: target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"

check:16'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

4: target triple = "arm-ti-none-eabi"

check:16'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:16'1 ? possible intended match

5:

check:16'0 ~

6: @g = local_unnamed_addr global i32 0, align 4

check:16'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

7:

check:16'0 ~

8: ; Function Attrs: nounwind

check:16'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~

9: define dso_local void @foo(i32 %n) local_unnamed_addr #0 !dbg !38 !prof !42 {

check:16'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.
.
.

--

Hello,
We are maintaining a downstream version of the monorepo based on the LLVM main branch. We have not transitioned to the new PM yet. In a recent attempt to merge the latest upstream commits into our monorepo we came across the following test failures after your commit.

Sorry about that, it's my fault for asking Sherwin to remove the -fexperimental-new-pass-manager flag. I've created https://reviews.llvm.org/D109956

The test patch worked and test is now passing again. Thanks for the quick help on this!