[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

2021-10-20 Thread Simon Pilgrim via Phabricator via cfe-commits 
RKSimon added inline comments.



Comment at: llvm/lib/LTO/LTOBackend.cpp:234
+  if (TM)
+TM->setPGOOption(PGOOpt);
 

@xur Why does TM need a null check - afaict it's already been dereferenced in 
the call tree, and will be again below.

Noticed in a scan-build warning: 
https://llvm.org/reports/scan-build/report-LTOBackend.cpp-runNewPMPasses-34-35dfe9.html#EndPath


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107878

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

2021-08-19 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits 
dexonsmith added inline comments.



Comment at: llvm/lib/CodeGen/MIRSampleProfile.cpp:289
+
+bool MIRProfileLoaderPass::runOnMachineFunction(MachineFunction &MF) {
+  if (!MIRSampleLoader->isValid())

JDevlieghere wrote:
> Why is this outside the `llvm` namespace? 
I think it's common style in LLVM to have function definitions outside of 
namespaces -- IMO, the odd thing here is that the preceding function 
definitions were inside the namespace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107878

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

2021-08-19 Thread Jonas Devlieghere via Phabricator via cfe-commits 
JDevlieghere added inline comments.



Comment at: llvm/include/llvm/CodeGen/MIRSampleProfile.h:59
+MIRSampleLoader(
+std::make_unique(FileName, RemappingFileName)) {
+LowBit = getFSPassBitBegin(P);

You're instantiating a forward-declared type. This breaks the modules build: 
https://green.lab.llvm.org/green/job/lldb-cmake/34450/console



Comment at: llvm/lib/CodeGen/MIRSampleProfile.cpp:289
+
+bool MIRProfileLoaderPass::runOnMachineFunction(MachineFunction &MF) {
+  if (!MIRSampleLoader->isValid())

Why is this outside the `llvm` namespace? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107878

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

2021-08-19 Thread Hongtao Yu via Phabricator via cfe-commits 
hoy added inline comments.



Comment at: llvm/test/CodeGen/X86/fsafdo_test2.ll:3
+; RUN: llvm-profdata merge --sample -profile-isfs -o %t.afdo 
%S/Inputs/fsloader.afdo
+; RUN: llc -enable-fs-discriminator -fs-profile-file=%t.afdo 
-show-fs-branchprob -disable-ra-fsprofile-loader=false 
-disable-layout-fsprofile-loader=false < %s 2>&1 | FileCheck %s 
--check-prefix=LOADER
+;

lenary wrote:
> I think this `REQUIRES: asserts` if you're checking the debug output?
That's right. It was just fixed by https://reviews.llvm.org/D108364.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107878

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

2021-08-19 Thread Wenlei He via Phabricator via cfe-commits 
wenlei added inline comments.



Comment at: llvm/test/CodeGen/X86/fsafdo_test2.ll:3
+; RUN: llvm-profdata merge --sample -profile-isfs -o %t.afdo 
%S/Inputs/fsloader.afdo
+; RUN: llc -enable-fs-discriminator -fs-profile-file=%t.afdo 
-show-fs-branchprob -disable-ra-fsprofile-loader=false 
-disable-layout-fsprofile-loader=false < %s 2>&1 | FileCheck %s 
--check-prefix=LOADER
+;

lenary wrote:
> I think this `REQUIRES: asserts` if you're checking the debug output?
Should be fixed in D108364


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107878

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

2021-08-19 Thread Sam Elliott via Phabricator via cfe-commits 
lenary added inline comments.



Comment at: llvm/test/CodeGen/X86/fsafdo_test2.ll:3
+; RUN: llvm-profdata merge --sample -profile-isfs -o %t.afdo 
%S/Inputs/fsloader.afdo
+; RUN: llc -enable-fs-discriminator -fs-profile-file=%t.afdo 
-show-fs-branchprob -disable-ra-fsprofile-loader=false 
-disable-layout-fsprofile-loader=false < %s 2>&1 | FileCheck %s 
--check-prefix=LOADER
+;

I think this `REQUIRES: asserts` if you're checking the debug output?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107878

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

2021-08-18 Thread Rong Xu via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5fdaaf7fd8f3: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) 
profile loader (authored by xur).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D107878?vs=367321&id=367381#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107878

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/CodeGen/MIRSampleProfile.h
  llvm/include/llvm/CodeGen/MachineDominators.h
  llvm/include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/include/llvm/Support/PGOOptions.h
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
  llvm/lib/CodeGen/CMakeLists.txt
  llvm/lib/CodeGen/MIRSampleProfile.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/test/CodeGen/X86/Inputs/fsloader.afdo
  llvm/test/CodeGen/X86/fsafdo_test2.ll
  llvm/tools/opt/NewPMDriver.cpp

Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -284,6 +284,9 @@
   P->CSAction = PGOOptions::CSIRUse;
 }
   }
+  if (TM)
+TM->setPGOOption(P);
+
   LoopAnalysisManager LAM;
   FunctionAnalysisManager FAM;
   CGSCCAnalysisManager CGAM;
Index: llvm/test/CodeGen/X86/fsafdo_test2.ll
===
--- llvm/test/CodeGen/X86/fsafdo_test2.ll
+++ llvm/test/CodeGen/X86/fsafdo_test2.ll
@@ -1,4 +1,7 @@
 ; RUN: llc -enable-fs-discriminator < %s | FileCheck %s
+; RUN: llvm-profdata merge --sample -profile-isfs -o %t.afdo %S/Inputs/fsloader.afdo
+; RUN: llc -enable-fs-discriminator -fs-profile-file=%t.afdo -show-fs-branchprob -disable-ra-fsprofile-loader=false -disable-layout-fsprofile-loader=false < %s 2>&1 | FileCheck %s --check-prefix=LOADER
+;
 ;;
 ;; C source code for the test (compiler at -O3):
 ;; // A test case for loop unroll.
@@ -50,6 +53,25 @@
 ; CHECK: .byte   1
 ; CHECK: .size   __llvm_fs_discriminator__, 1
 
+;; Check that new branch probs are generated.
+; LOADER: Set branch fs prob: MBB (1 -> 3): unroll.c:22:11-->unroll.c:24:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x7aca7894 / 0x8000 = 95.93%
+; LOADER: Set branch fs prob: MBB (1 -> 2): unroll.c:22:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x0535876c / 0x8000 = 4.07%
+; LOADER: Set branch fs prob: MBB (3 -> 5): unroll.c:24:11-->unroll.c:22:11 W=283590  0x3000 / 0x8000 = 37.50% --> 0x7aca7894 / 0x8000 = 95.93%
+; LOADER: Set branch fs prob: MBB (3 -> 4): unroll.c:24:11 W=283590  0x5000 / 0x8000 = 62.50% --> 0x0535876c / 0x8000 = 4.07%
+; LOADER: Set branch fs prob: MBB (5 -> 8): unroll.c:22:11-->unroll.c:24:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x021c112e / 0x8000 = 1.65%
+; LOADER: Set branch fs prob: MBB (5 -> 7): unroll.c:22:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x7de3eed2 / 0x8000 = 98.35%
+; LOADER: Set branch fs prob: MBB (8 -> 10): unroll.c:24:11-->unroll.c:22:11 W=283590  0x3000 / 0x8000 = 37.50% --> 0x / 0x8000 = 0.00%
+; LOADER: Set branch fs prob: MBB (8 -> 9): unroll.c:24:11 W=283590  0x5000 / 0x8000 = 62.50% --> 0x8000 / 0x8000 = 100.00%
+; LOADER: Set branch fs prob: MBB (10 -> 12): unroll.c:22:11-->unroll.c:24:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x7aca7894 / 0x8000 = 95.93%
+; LOADER: Set branch fs prob: MBB (10 -> 11): unroll.c:22:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x0535876c / 0x8000 = 4.07%
+; LOADER: Set branch fs prob: MBB (12 -> 14): unroll.c:24:11-->unroll.c:22:11 W=283590  0x3000 / 0x8000 = 37.50% --> 0x02012507 / 0x8000 = 1.57%
+; LOADER: Set branch fs prob: MBB (12 -> 13): unroll.c:24:11 W=283590  0x5000 / 0x8000 = 62.50% --> 0x7dfedaf9 / 0x8000 = 98.43%
+; LOADER: Set branch fs prob: MBB (14 -> 16): unroll.c:22:11-->unroll.c:24:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x0a5856e1 / 0x8000 = 8.08%
+; LOADER: Set branch fs prob: MBB (14 -> 15): unroll.c:22:11 W=283590  0x4000 / 0x8000 = 50.00% --> 0x75a7a91f / 0x8000 = 91.92%
+; LOADER: Set branch fs prob: MBB (16 -> 18): unroll.c:24:11-->unroll.c:19:3 W=283590  0x3000 / 0x8000 = 37.50% --> 0x16588166 / 0x8000 = 17.46%
+; LOADER: Set branch fs prob: MBB (16 -> 17): unroll.c:24:11 W=283590  0x5000 / 0x8000 = 62.50% --> 0x69a77e9a / 0x8000 = 82.54%
+
+
 target triple = "x86_64-unknown-linux-gnu"
 
 @sum = dso_local local_unnamed_addr