This is an archive of the discontinued LLVM Phabricator instance.

[PGO] Add PGO support at -O0 in the experimental new pass manager
ClosedPublic

Authored by xur on Jul 1 2019, 12:23 PM.

Details

Summary

Add PGO support at -O0 in the experimental new pass manager to sync the behavior with the legacy pass manager.

Also change the test of gcc-flag-compatibility.c for more complete test:
(1) change the match string to "profc" and "profd" to ensure the instrumentation is happening.
(2) add IR format proftext so that PGO use is tested.

Diff Detail

Repository
rL LLVM

Event Timeline

xur created this revision.Jul 1 2019, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 12:23 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
xur updated this revision to Diff 207392.Jul 1 2019, 12:59 PM
xur marked an inline comment as done.

Sent the wrong test file in last patch.
Update to the correct one.

xur added a comment.Jul 9 2019, 10:58 AM

Ping

llvm/lib/Passes/PassBuilder.cpp
576 ↗(On Diff #207389)

I moved this pass under the condition when Early inline is enabled. I'm under the impression that the intention is to clean up the code for the inline.
Chandler: you added this pass. If you don't like this move, I can pull it out and put it under another (Level > 0) condition.

chandlerc requested changes to this revision.Jul 11 2019, 5:26 PM

Sorry for the delay here.

It'd be nice to land the LLVM patch first and with its own testing -- we should have testing for the pass builder independent of Clang (IE, in the LLVM tests).

One option would be to test it with a unittest, not sure we have pass builder unittests at the moment.

Probably a better option would be to define a pseudo pass name like our default<O0>, maybe pgo<O0>. Then you can parse the O0 the same way we do for the default thing. when it is O0 you can call the new routine I've suggested below. When it is higher, you can call the existing routine much like the default pipeline code does. This would all be implemented inside the pass builder so no issues w/ using the utility code there.

This would let us much more nicely write tests for the pipeline fragment generated for PGO both at O0 and above -- we have tests that specifically print out the pipeline sequence. This makes it very easy to visualize changes to the pipeline. And it would let you test the O0 code path here.

The clang part of the patch (once adapted to the narrower API) seems likely to be good then.

I'm happy for this to be two patches (first llvm, then Clang), or one patch. Whatever is easier for you.

llvm/lib/Passes/PassBuilder.cpp
579–602 ↗(On Diff #207392)

Rather than moving the entire routine that is really only intended for the innards of the pass manager to be public, I'd do something a bit more narrow...

Maybe just add pipeline method to the pass builder to generate a minimal pipeline suitable for O0 usage?

I think this code will be simpler to read w/o having to have the O0 conditions plumbed all the way through it, and the duplication is tiny.

If you want, could pull out the use bits which are common and use early exit to make the code more clear:

if (!RunProfileGen) {
  addPGOUsePasses(...);
  return;
}

...

(Not sure what name to use for this, but you see the idea.) Then there would be almost no duplication between the O0 and this code, w/o having to have the conditions threaded throughout.

576 ↗(On Diff #207389)

This makes sense to me. It was probably just transcription error on my part.

This revision now requires changes to proceed.Jul 11 2019, 5:26 PM
xur marked 3 inline comments as done.Jul 29 2019, 3:21 PM

I'm sorry that I missed this review for this long!

Sorry for the delay here.

It'd be nice to land the LLVM patch first and with its own testing -- we should have testing for the pass builder independent of Clang (IE, in the LLVM tests).

One option would be to test it with a unittest, not sure we have pass builder unittests at the moment.

I initially had the pipeline change for this. But clang did not use the default pipeline, so I removed that change.
But this makes sense now to me with a unittest.

Probably a better option would be to define a pseudo pass name like our default<O0>, maybe pgo<O0>. Then you can parse the O0 the same way we do for the default thing. when it is O0 you can call the new routine I've suggested below. When it is higher, you can call the existing routine much like the default pipeline code does. This would all be implemented inside the pass builder so no issues w/ using the utility code there.

This would let us much more nicely write tests for the pipeline fragment generated for PGO both at O0 and above -- we have tests that specifically print out the pipeline sequence. This makes it very easy to visualize changes to the pipeline. And it would let you test the O0 code path here.

hmm. I think PGO is orthogonal to default pipeline: we now have prefixes of "default", "thinlto-pre-link", "thinlto", "lto", "lto-pre-link". They all can work with PGO enabled. It seems to me PGO<O0> will not cover all of them.

The clang part of the patch (once adapted to the narrower API) seems likely to be good then.

I'm happy for this to be two patches (first llvm, then Clang), or one patch. Whatever is easier for you.

Great! I would prefer to have only one patch -- that would be easier.

Sorry for the delay here.

It'd be nice to land the LLVM patch first and with its own testing -- we should have testing for the pass builder independent of Clang (IE, in the LLVM tests).

One option would be to test it with a unittest, not sure we have pass builder unittests at the moment.

Probably a better option would be to define a pseudo pass name like our default<O0>, maybe pgo<O0>. Then you can parse the O0 the same way we do for the default thing. when it is O0 you can call the new routine I've suggested below. When it is higher, you can call the existing routine much like the default pipeline code does. This would all be implemented inside the pass builder so no issues w/ using the utility code there.

This would let us much more nicely write tests for the pipeline fragment generated for PGO both at O0 and above -- we have tests that specifically print out the pipeline sequence. This makes it very easy to visualize changes to the pipeline. And it would let you test the O0 code path here.

The clang part of the patch (once adapted to the narrower API) seems likely to be good then.

I'm happy for this to be two patches (first llvm, then Clang), or one patch. Whatever is easier for you.

llvm/lib/Passes/PassBuilder.cpp
579–602 ↗(On Diff #207392)

I added a new function named as "addPGOInstrPassesForO0" which only does the O0 passes for PGO. It is a public function as it will be called from clang.
Also followed the advice to refactor the code for better readability.

576 ↗(On Diff #207389)

Got it.

xur updated this revision to Diff 212237.Jul 29 2019, 3:25 PM
xur marked an inline comment as done.

Integrated Chandler's review comments.

chandlerc accepted this revision.Aug 1 2019, 2:40 PM

LGTM with two nits addressed, thanks!

clang/lib/CodeGen/BackendUtil.cpp
1125 ↗(On Diff #212237)

Minor nit, here and else where with the comment-named bools I'd use the style: /*RunProfileGen=*/PGOOpt->Action == PGOOPtions::IRInstr

This was suggested as more consistent w/ Clang style, and it seems a bit cleaner. Also, clang-tidy will recognize and check that the comment uses the same name as the parameter.

llvm/lib/Passes/PassBuilder.cpp
1839 ↗(On Diff #212237)

Same nit about the parameter comments here.

This revision is now accepted and ready to land.Aug 1 2019, 2:40 PM
xur marked 3 inline comments as done.Aug 1 2019, 2:57 PM
xur added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
1125 ↗(On Diff #212237)

Thanks for the suggestion and the reasoning.

I actually wrote it in one line.
This was produced by "clang/tools/clang-format/clang-format-diff.py". :-)

I'm using now "/*RunProfileGen=*/ (PGOOpt->Action == PGOOPtions::IRInstr)" so clang-format-diff.py won't complain

This revision was automatically updated to reflect the committed changes.
xur marked an inline comment as done.
phosek added a subscriber: phosek.Aug 1 2019, 11:51 PM

Looks like this change broke Profile/gcc-flag-compatibility.c test on macOS:

******************** TEST 'Clang :: Profile/gcc-flag-compatibility.c' FAILED ********************
Script:
--
: 'RUN: at line 10';   /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c -c -S -o - -emit-llvm -fprofile-generate -fno-experimental-new-pass-manager | /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/FileCheck -check-prefix=PROFILE-GEN /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c
: 'RUN: at line 11';   /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c -c -S -o - -emit-llvm -fprofile-generate -fexperimental-new-pass-manager | /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/FileCheck -check-prefix=PROFILE-GEN /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c
: 'RUN: at line 16';   /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c -c -S -o - -emit-llvm -fprofile-generate=/path/to -fno-experimental-new-pass-manager | /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/FileCheck -check-prefixes=PROFILE-GEN,PROFILE-GEN-EQ /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c
: 'RUN: at line 22';   rm -rf /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir
: 'RUN: at line 23';   mkdir -p /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path
: 'RUN: at line 24';   llvm-profdata merge /b/s/w/ir/k/llvm-project/clang/test/Profile/Inputs/gcc-flag-compatibility.proftext -o /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path/default.profdata
: 'RUN: at line 25';   /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c -o - -Xclang -disable-llvm-passes -emit-llvm -S -fprofile-use=/b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path -fno-experimental-new-pass-manager | /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/FileCheck -check-prefix=PROFILE-USE /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c
: 'RUN: at line 26';   /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c -o - -Xclang -disable-llvm-passes -emit-llvm -S -fprofile-use=/b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path -fexperimental-new-pass-manager | /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/FileCheck -check-prefix=PROFILE-USE /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c
: 'RUN: at line 30';   rm -rf /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir
: 'RUN: at line 31';   mkdir -p /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path
: 'RUN: at line 32';   llvm-profdata merge /b/s/w/ir/k/llvm-project/clang/test/Profile/Inputs/gcc-flag-compatibility.proftext -o /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path/file.prof
: 'RUN: at line 33';   /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c -o - -Xclang -disable-llvm-passes -emit-llvm -S -fprofile-use=/b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path/file.prof -fno-experimental-new-pass-manager | /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/FileCheck -check-prefix=PROFILE-USE /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c
: 'RUN: at line 34';   /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c -o - -Xclang -disable-llvm-passes -emit-llvm -S -fprofile-use=/b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path/file.prof -fexperimental-new-pass-manager | /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/FileCheck -check-prefix=PROFILE-USE /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c
: 'RUN: at line 39';   rm -rf /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir
: 'RUN: at line 40';   mkdir -p /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path
: 'RUN: at line 41';   llvm-profdata merge /b/s/w/ir/k/llvm-project/clang/test/Profile/Inputs/gcc-flag-compatibility_IR.proftext -o /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path/default.profdata
: 'RUN: at line 42';   /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c -o - -emit-llvm -S -fprofile-use=/b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path -fno-experimental-new-pass-manager | /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/FileCheck -check-prefix=PROFILE-USE-IR /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c
: 'RUN: at line 43';   /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c -o - -emit-llvm -S -fprofile-use=/b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path -fexperimental-new-pass-manager | /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/FileCheck -check-prefix=PROFILE-USE-IR /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c
: 'RUN: at line 47';   rm -rf /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir
: 'RUN: at line 48';   mkdir -p /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path
: 'RUN: at line 49';   llvm-profdata merge /b/s/w/ir/k/llvm-project/clang/test/Profile/Inputs/gcc-flag-compatibility_IR.proftext -o /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path/file.prof
: 'RUN: at line 50';   /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c -o - -emit-llvm -S -fprofile-use=/b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path/file.prof -fno-experimental-new-pass-manager | /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/FileCheck -check-prefix=PROFILE-USE-IR /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c
: 'RUN: at line 51';   /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/clang /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c -o - -emit-llvm -S -fprofile-use=/b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/tools/clang/test/Profile/Output/gcc-flag-compatibility.c.tmp.dir/some/path/file.prof -fexperimental-new-pass-manager | /b/s/w/ir/k/recipe_cleanup/clangOLc9jG/llvm_build_dir/bin/FileCheck -check-prefix=PROFILE-USE-IR /b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c:12:17: error: PROFILE-GEN: expected string not found in input
// PROFILE-GEN: @__profc_main = private global [2 x i64] zeroinitializer, section "__llvm_prf_cnts", align 8
                ^
<stdin>:1:1: note: scanning from here
; ModuleID = '/b/s/w/ir/k/llvm-project/clang/test/Profile/gcc-flag-compatibility.c'
^
<stdin>:9:1: note: possible intended match here
@__profc_main = private global [2 x i64] zeroinitializer, section "__DATA,__llvm_prf_cnts", align 8
^

--