[llvm] [clang-tools-extra] [clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2024-02-05 Thread David Li via cfe-commits 

david-xl wrote:

> > > > I don't understand, if you're saying the profile is accurate, then 
> > > > those functions are actually cold, so we should be able to mark them as 
> > > > optsize?
> > > 
> > > 
> > > Accurate is not black or white. The current heuristic requires certain 
> > > level of accuracy to be effective. If you make the heuristics more 
> > > aggressive (like what this patch is doing), you're raising the 
> > > requirement of what can be considered accurate, and profile not meeting 
> > > that new requirement could see regression with new heuristic.
> > > Whether a function is cold or not also depends on what is the calling 
> > > context and how inlining is done. All that makes function level 
> > > annotation inherently inaccurate when done before inlining. Not that we 
> > > shouldn't try it, but it's not as clear cut as it appears to be, and we 
> > > need to be careful.
> > 
> > 
> > It will be more conservative (pre-inlining), so won't cause additional 
> > optimization suppression compared with the current PGSO.
> 
> Sample PGO profile has inline context, so in the profile, we may have `foo` 
> as cold and `bar->foo` as hot, but if later inliner rejects `bar->foo` 
> inlining, `foo` can be hot. So marking `foo` as cold pre-inline can still be 
> inaccurate (and not conservative).
> 
> In this PR, you have `PGOColdFuncAttr` default to `ColdFuncOpt::Default`. As 
> long as you keep it that way, this PR is fine for sample pgo (it's no-op 
> unless `pgo-cold-func-opt` is used explicitly).

Good example. This pass should be run post-inline.  @aeubanks, any reason we 
want to run it early in the pipeline?

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang-tools-extra] [clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2024-02-05 Thread David Li via cfe-commits 

david-xl wrote:

> > I don't understand, if you're saying the profile is accurate, then those 
> > functions are actually cold, so we should be able to mark them as optsize?
> 
> Accurate is not black or white. The current heuristic requires certain level 
> of accuracy to be effective. If you make the heuristics more aggressive (like 
> what this patch is doing), you're raising the requirement of what can be 
> considered accurate, and profile not meeting that new requirement could see 
> regression with new heuristic.
> 
> Whether a function is cold or not also depends on what is the calling context 
> and how inlining is done. All that makes function level annotation inherently 
> inaccurate when done before inlining. Not that we shouldn't try it, but it's 
> not as clear cut as it appears to be, and we need to be careful.

It will be more conservative (pre-inlining), so won't cause additional 
optimization suppression compared with the current PGSO.

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [llvm] [clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2024-02-05 Thread via cfe-commits 

WenleiHe wrote:

> I don't understand, if you're saying the profile is accurate, then those 
> functions are actually cold, so we should be able to mark them as optsize?

Accurate is not black or white. The current heuristic requires certain level of 
accuracy to be effective. If you make the heuristics more aggressive (like what 
this patch is doing), you're raising the requirement of what can be considered 
accurate, and profile not meeting that new requirement could see regression 
with new heuristic. 

Whether a function is cold or not also depends on what is the calling context 
and how inlining is done. All that makes function level annotation inherently 
inaccurate when done before inlining. Not that we shouldn't try it, but it's 
not as clear cut as it appears to be, and we need to be careful. 

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2024-02-05 Thread Arthur Eubanks via cfe-commits 


@@ -0,0 +1,73 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Transforms/Instrumentation/PGOForceFunctionAttrs.h"
+#include "llvm/Analysis/BlockFrequencyInfo.h"
+#include "llvm/Analysis/ProfileSummaryInfo.h"
+#include "llvm/IR/PassManager.h"
+#include "llvm/Support/ErrorHandling.h"
+
+using namespace llvm;
+
+static bool shouldRunOnFunction(Function &F, ProfileSummaryInfo &PSI,
+FunctionAnalysisManager &FAM) {
+  if (F.hasFnAttribute(Attribute::Cold))
+return true;
+  if (!PSI.hasProfileSummary())
+return false;
+  BlockFrequencyInfo &BFI = FAM.getResult(F);
+  return PSI.isFunctionColdInCallGraph(&F, BFI);
+}
+
+PreservedAnalyses PGOForceFunctionAttrsPass::run(Module &M,
+ ModuleAnalysisManager &AM) {
+  if (ColdType == PGOOptions::ColdFuncOpt::Default)
+return PreservedAnalyses::all();
+  ProfileSummaryInfo &PSI = AM.getResult(M);
+  FunctionAnalysisManager &FAM =
+  AM.getResult(M).getManager();
+  bool MadeChange = false;
+  for (Function &F : M) {
+if (F.isDeclaration())
+  continue;
+if (!shouldRunOnFunction(F, PSI, FAM))
+  continue;
+// Add optsize/minsize/optnone if requested.
+switch (ColdType) {
+case PGOOptions::ColdFuncOpt::Default:
+  llvm_unreachable("bailed out for default above");
+  break;
+case PGOOptions::ColdFuncOpt::OptSize:
+  if (!F.hasFnAttribute(Attribute::OptimizeNone) &&
+  !F.hasFnAttribute(Attribute::OptimizeForSize) &&
+  !F.hasFnAttribute(Attribute::MinSize)) {
+F.addFnAttr(Attribute::OptimizeForSize);
+MadeChange = true;
+  }
+  break;
+case PGOOptions::ColdFuncOpt::MinSize:
+  // Change optsize to minsize.
+  if (!F.hasFnAttribute(Attribute::OptimizeNone) &&
+  !F.hasFnAttribute(Attribute::MinSize)) {
+F.removeFnAttr(Attribute::OptimizeForSize);

aeubanks wrote:

it was intentional for optnone to take precedence, but I've changed it to bail 
out if we see optnone/minsize/optsize at all

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2024-02-05 Thread Arthur Eubanks via cfe-commits 

aeubanks wrote:

> FWIW we've tried this with sampling PGO in the past. While on paper this 
> seems like an obvious thing to do, in reality aggressively tuning down opt 
> level for cold functions can lead to regression since profile isn't always 
> accurate.
> 
> That said, as long as this change only provides options for users to make 
> decision and not changing the default behavior, it's probably fine.

IIUC, this won't affect sample profile unless you mark the sample profile as 
"accurate" (e.g. `-profile-sample-accurate`). But I should double check.

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2024-02-05 Thread Arthur Eubanks via cfe-commits 

aeubanks wrote:

> How does this relate to the existing `shouldOptimizeForSize(Function&, ...)` 
> and `shouldOptimizeForSize(MachineFunction&, ...)` APIs which appear to 
> provide similar functionality at a first glance. If they are the same, then 
> we should have a plan in place to cleanup and only have one system 
> afterwards, if there are important differences, then I wouldn't mind some 
> comments explaining them.

This is intended to replace `shouldOptimizeForSize()`. We've seen multiple 
cases of calls to `shouldOptimizeForSize()` blowing up compile times if we're 
not being careful with the calls to it, since it ends up calling expensive 
profile information code. The replacement is to just check if the function has 
the `optsize`/`minsize` attribute. I'll mention this in the description.

The basic block versions of these should remain as they are since they actually 
do need to look at profile information to determine per-block information.

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2024-01-30 Thread via cfe-commits 

WenleiHe wrote:

> > How does this relate to the existing `shouldOptimizeForSize(Function&, 
> > ...)` and `shouldOptimizeForSize(MachineFunction&, ...)` APIs which appear 
> > to provide similar functionality at a first glance. If they are the same, 
> > then we should have a plan in place to cleanup and only have one system 
> > afterwards, if there are important differences, then I wouldn't mind some 
> > comments explaining them.
> 
> This patch allows more user control on the cold function action types.
> 
> Another difference is that the existing API can be invoked in postInline 
> passes which can see more cold functions after inlining. To replace those 
> APIs, the new pass will need to run again post-inlining.

They provide similar but different controls. However, the fact that the two 
mechanisms are completely disconnected is not ideal. If we want to apply an opt 
strategy for cold code, we should do it at both function level and block level 
consistently. 

In fact the unification was also discussed on the original RFC: 
https://discourse.llvm.org/t/rfc-new-feature-proposal-de-optimizing-cold-functions-using-pgo-info/56388/16?u=wenleihe
  Is there a plan on that front? 

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2024-01-30 Thread via cfe-commits 


@@ -0,0 +1,73 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Transforms/Instrumentation/PGOForceFunctionAttrs.h"
+#include "llvm/Analysis/BlockFrequencyInfo.h"
+#include "llvm/Analysis/ProfileSummaryInfo.h"
+#include "llvm/IR/PassManager.h"
+#include "llvm/Support/ErrorHandling.h"
+
+using namespace llvm;
+
+static bool shouldRunOnFunction(Function &F, ProfileSummaryInfo &PSI,
+FunctionAnalysisManager &FAM) {
+  if (F.hasFnAttribute(Attribute::Cold))
+return true;
+  if (!PSI.hasProfileSummary())
+return false;
+  BlockFrequencyInfo &BFI = FAM.getResult(F);
+  return PSI.isFunctionColdInCallGraph(&F, BFI);
+}
+
+PreservedAnalyses PGOForceFunctionAttrsPass::run(Module &M,
+ ModuleAnalysisManager &AM) {
+  if (ColdType == PGOOptions::ColdFuncOpt::Default)
+return PreservedAnalyses::all();
+  ProfileSummaryInfo &PSI = AM.getResult(M);
+  FunctionAnalysisManager &FAM =
+  AM.getResult(M).getManager();
+  bool MadeChange = false;
+  for (Function &F : M) {
+if (F.isDeclaration())
+  continue;
+if (!shouldRunOnFunction(F, PSI, FAM))
+  continue;
+// Add optsize/minsize/optnone if requested.
+switch (ColdType) {
+case PGOOptions::ColdFuncOpt::Default:
+  llvm_unreachable("bailed out for default above");
+  break;
+case PGOOptions::ColdFuncOpt::OptSize:
+  if (!F.hasFnAttribute(Attribute::OptimizeNone) &&
+  !F.hasFnAttribute(Attribute::OptimizeForSize) &&
+  !F.hasFnAttribute(Attribute::MinSize)) {
+F.addFnAttr(Attribute::OptimizeForSize);
+MadeChange = true;
+  }
+  break;
+case PGOOptions::ColdFuncOpt::MinSize:
+  // Change optsize to minsize.
+  if (!F.hasFnAttribute(Attribute::OptimizeNone) &&
+  !F.hasFnAttribute(Attribute::MinSize)) {
+F.removeFnAttr(Attribute::OptimizeForSize);

WenleiHe wrote:

Inclined to say that we should honor existing attributes which may come from 
source annotations (optnone can be used for correctness reasons, e.g. 
workaround bugs etc). For this reason, I'm thinking about it as 
`PGODerivedFunctionAttrs` instead of `PGOForceFunctionAttrs`.

I'm also seeing inconsistency here in how different attributes are handled, 
e.g. `OptNone` seems to take precedence over any existing attributes, but not 
the case for some others. 

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2024-01-30 Thread Matthias Braun via cfe-commits 

MatzeB wrote:

How does this relate to the existing `shouldOptimizeForSize(Function&)` and 
similar APIs which appear to provide similar functionality at a first glance. 
If they are the same, then we should have a plan in place to cleanup and only 
have one system afterwards, if there are important differences, then I wouldn't 
mind some comments getting added :)

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2024-01-30 Thread David Li via cfe-commits 


@@ -0,0 +1,73 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Transforms/Instrumentation/PGOForceFunctionAttrs.h"
+#include "llvm/Analysis/BlockFrequencyInfo.h"
+#include "llvm/Analysis/ProfileSummaryInfo.h"
+#include "llvm/IR/PassManager.h"
+#include "llvm/Support/ErrorHandling.h"
+
+using namespace llvm;
+
+static bool shouldRunOnFunction(Function &F, ProfileSummaryInfo &PSI,
+FunctionAnalysisManager &FAM) {
+  if (F.hasFnAttribute(Attribute::Cold))
+return true;
+  if (!PSI.hasProfileSummary())
+return false;
+  BlockFrequencyInfo &BFI = FAM.getResult(F);
+  return PSI.isFunctionColdInCallGraph(&F, BFI);
+}
+
+PreservedAnalyses PGOForceFunctionAttrsPass::run(Module &M,
+ ModuleAnalysisManager &AM) {
+  if (ColdType == PGOOptions::ColdFuncOpt::Default)
+return PreservedAnalyses::all();
+  ProfileSummaryInfo &PSI = AM.getResult(M);
+  FunctionAnalysisManager &FAM =
+  AM.getResult(M).getManager();
+  bool MadeChange = false;
+  for (Function &F : M) {
+if (F.isDeclaration())
+  continue;
+if (!shouldRunOnFunction(F, PSI, FAM))
+  continue;
+// Add optsize/minsize/optnone if requested.
+switch (ColdType) {
+case PGOOptions::ColdFuncOpt::Default:
+  llvm_unreachable("bailed out for default above");
+  break;
+case PGOOptions::ColdFuncOpt::OptSize:
+  if (!F.hasFnAttribute(Attribute::OptimizeNone) &&
+  !F.hasFnAttribute(Attribute::OptimizeForSize) &&
+  !F.hasFnAttribute(Attribute::MinSize)) {
+F.addFnAttr(Attribute::OptimizeForSize);
+MadeChange = true;
+  }
+  break;
+case PGOOptions::ColdFuncOpt::MinSize:
+  // Change optsize to minsize.
+  if (!F.hasFnAttribute(Attribute::OptimizeNone) &&
+  !F.hasFnAttribute(Attribute::MinSize)) {
+F.removeFnAttr(Attribute::OptimizeForSize);

david-xl wrote:

should it overwrite the existing attribute?

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-19 Thread David Li via cfe-commits 


@@ -0,0 +1,65 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Transforms/Instrumentation/MarkColdFunctions.h"
+#include "llvm/Analysis/BlockFrequencyInfo.h"
+#include "llvm/Analysis/ProfileSummaryInfo.h"
+#include "llvm/IR/PassManager.h"
+
+using namespace llvm;
+
+PreservedAnalyses MarkColdFunctionsPass::run(Module &M,
+ ModuleAnalysisManager &AM) {
+  if (ColdType == PGOOptions::ColdFuncAttr::None)
+return PreservedAnalyses::all();
+  ProfileSummaryInfo &PSI = AM.getResult(M);
+  if (!PSI.hasProfileSummary())
+return PreservedAnalyses::all();
+  FunctionAnalysisManager &FAM =
+  AM.getResult(M).getManager();
+  bool MadeChange = false;
+  for (Function &F : M) {
+if (F.isDeclaration())
+  continue;
+BlockFrequencyInfo &BFI = FAM.getResult(F);
+if (!PSI.isFunctionColdInCallGraph(&F, BFI))

david-xl wrote:

The same optimization strategy should also be applied to function already 
marked with cold attribute (either by user or previous passes).

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-16 Thread Ellis Hoag via cfe-commits 


@@ -0,0 +1,28 @@
+//===- MarkColdFunctions.h - *- C++ 
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_TRANSFORMS_INSTRUMENTATION_MARKCOLDFUNCTIONS_H
+#define LLVM_TRANSFORMS_INSTRUMENTATION_MARKCOLDFUNCTIONS_H
+
+#include "llvm/IR/PassManager.h"
+#include "llvm/Support/PGOOptions.h"
+
+namespace llvm {
+
+struct MarkColdFunctionsPass : public PassInfoMixin {

ellishg wrote:

I think this pass could be generalized a bit. It seems that the goal for this 
pr is to reduce the size overhead of `-O2` by forcing cold functions to be 
`optsize`/`minsize`. Another goal could be to improve the performance of `-Oz` 
by forcing very hot functions to be `optsize` (as opposed to `minsize`) or 
remove `optsize`/`minsize` from them entirely. This is actually something we 
explored in [this paper](https://dl.acm.org/doi/10.1145/3497776.3517764) a 
while back and I would eventually like to upstream something similar. I believe 
this could be incorporated into this pass, but the name seems to be too vague. 
I think "PGO" should to be in the name so users know that it is involved. Maybe 
`PGOForceFuncAttrsPass` or `ProfileGuidedFuncAttrsPass`?

CC @kyulee-com 

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-16 Thread Ellis Hoag via cfe-commits 


@@ -1127,6 +1134,11 @@ 
PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   if (EnableSyntheticCounts && !PGOOpt)
 MPM.addPass(SyntheticCountsPropagation());
 
+  if (EnableMarkColdFunctions && PGOOpt &&
+  (PGOOpt->Action == PGOOptions::SampleUse ||
+   PGOOpt->Action == PGOOptions::IRUse))

ellishg wrote:

It seems that this pass isn't enabled for CS profiles. Can they benefit from 
this pass?

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-16 Thread David Li via cfe-commits 


@@ -0,0 +1,65 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Transforms/Instrumentation/MarkColdFunctions.h"
+#include "llvm/Analysis/BlockFrequencyInfo.h"
+#include "llvm/Analysis/ProfileSummaryInfo.h"
+#include "llvm/IR/PassManager.h"
+
+using namespace llvm;
+
+PreservedAnalyses MarkColdFunctionsPass::run(Module &M,

david-xl wrote:

yes, you can use profile-sample-accurate or profile-accurate-for-syminlist to 
control the behavior, similar to how AFDO is selecting functions to put in 
.text.unlikely.

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-16 Thread Arthur Eubanks via cfe-commits 


@@ -0,0 +1,65 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Transforms/Instrumentation/MarkColdFunctions.h"
+#include "llvm/Analysis/BlockFrequencyInfo.h"
+#include "llvm/Analysis/ProfileSummaryInfo.h"
+#include "llvm/IR/PassManager.h"
+
+using namespace llvm;
+
+PreservedAnalyses MarkColdFunctionsPass::run(Module &M,

aeubanks wrote:

Having a global option determine how to handle `cold` makes the IR more 
complicated. Reusing something that already exists and is being used is nicer.

Another alternative is to rely on the profile loaders to mark functions as 
`cold`, then this pass only looks at functions marked `cold`. WDYT?

It looks like iFDO marks cold functions as `cold`, but sample FDO doesn't. If 
sample FDO is inaccurate then marking functions as `cold` may be bad, so I 
assume that's why we don't mark functions as `cold` for sample FDO. But there's 
also the "accurate" flag that says that the sample profile is accurate and 
functions not in the profile are actually cold. In that case we should mark 
functions as `cold`?

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-13 Thread David Li via cfe-commits 


@@ -202,6 +202,18 @@ static cl::opt
  cl::desc("Path to the profile remapping file."),
  cl::Hidden);
 
+static cl::opt PGOColdFuncAttr(
+"pgo-cold-func-attr", cl::init(PGOOptions::ColdFuncAttr::None), cl::Hidden,

david-xl wrote:

pgo-cold-func-opt

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-13 Thread David Li via cfe-commits 


@@ -0,0 +1,65 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Transforms/Instrumentation/MarkColdFunctions.h"
+#include "llvm/Analysis/BlockFrequencyInfo.h"
+#include "llvm/Analysis/ProfileSummaryInfo.h"
+#include "llvm/IR/PassManager.h"
+
+using namespace llvm;
+
+PreservedAnalyses MarkColdFunctionsPass::run(Module &M,

david-xl wrote:

why not just mark those function with 'cold' attribute, and have one global 
option to determine the optimization strategy of cold attributed functions? 
These functions are not different from functions annotated by a developer.

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-13 Thread David Li via cfe-commits 


@@ -27,10 +27,12 @@ class FileSystem;
 struct PGOOptions {
   enum PGOAction { NoAction, IRInstr, IRUse, SampleUse };
   enum CSPGOAction { NoCSAction, CSIRInstr, CSIRUse };
+  enum class ColdFuncAttr { None, OptSize, MinSize, OptNone };

david-xl wrote:

None --> Default

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-13 Thread David Li via cfe-commits 


@@ -44,6 +46,7 @@ struct PGOOptions {
   std::string MemoryProfile;
   PGOAction Action;
   CSPGOAction CSAction;
+  ColdFuncAttr ColdType;

david-xl wrote:

ColdType --> ColdOptType

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-13 Thread David Li via cfe-commits 


@@ -27,10 +27,12 @@ class FileSystem;
 struct PGOOptions {
   enum PGOAction { NoAction, IRInstr, IRUse, SampleUse };
   enum CSPGOAction { NoCSAction, CSIRInstr, CSIRUse };
+  enum class ColdFuncAttr { None, OptSize, MinSize, OptNone };

david-xl wrote:

Even though the optimization directive is implemented as a function attribute, 
it might be better to express the real intention is to decide 'optimization 
strategy', so perhaps ColdFuncOpt as name or ColdFuncOptAttr? ColdFuncAttr 
feels too broad.

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-13 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-lto

Author: Arthur Eubanks (aeubanks)


Changes

The performance of cold functions shouldn't matter too much, so if we care 
about binary sizes, add an option to mark cold functions as optsize/minsize for 
binary size, or optnone for compile times [1]. Clang patch will be in a future 
patch

Initial version: https://reviews.llvm.org/D149800

[1] 
https://discourse.llvm.org/t/rfc-new-feature-proposal-de-optimizing-cold-functions-using-pgo-info/56388


---

Patch is 22.23 KiB, truncated to 20.00 KiB below, full version: 
https://github.com/llvm/llvm-project/pull/69030.diff


13 Files Affected:

- (modified) clang/lib/CodeGen/BackendUtil.cpp (+12-6) 
- (modified) llvm/include/llvm/Support/PGOOptions.h (+3) 
- (added) llvm/include/llvm/Transforms/Instrumentation/MarkColdFunctions.h 
(+28) 
- (modified) llvm/lib/LTO/LTOBackend.cpp (+8-4) 
- (modified) llvm/lib/Passes/PassBuilder.cpp (+2-1) 
- (modified) llvm/lib/Passes/PassBuilderPipelines.cpp (+12) 
- (modified) llvm/lib/Passes/PassRegistry.def (+1) 
- (modified) llvm/lib/Support/PGOOptions.cpp (+4-3) 
- (modified) llvm/lib/Transforms/Instrumentation/CMakeLists.txt (+1) 
- (added) llvm/lib/Transforms/Instrumentation/MarkColdFunctions.cpp (+65) 
- (added) llvm/test/Transforms/MarkColdFunctions/basic.ll (+97) 
- (modified) llvm/tools/opt/NewPMDriver.cpp (+19-5) 
- (modified) 
llvm/utils/gn/secondary/llvm/lib/Transforms/Instrumentation/BUILD.gn (+1) 


``diff
diff --git a/clang/lib/CodeGen/BackendUtil.cpp 
b/clang/lib/CodeGen/BackendUtil.cpp
index d066819871dfde3..f0c9b9d4daf08b5 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -770,7 +770,8 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
 CodeGenOpts.InstrProfileOutput.empty() ? getDefaultProfileGenName()
: 
CodeGenOpts.InstrProfileOutput,
 "", "", CodeGenOpts.MemoryProfileUsePath, nullptr, PGOOptions::IRInstr,
-PGOOptions::NoCSAction, CodeGenOpts.DebugInfoForProfiling,
+PGOOptions::NoCSAction, PGOOptions::ColdFuncAttr::None,
+CodeGenOpts.DebugInfoForProfiling,
 /*PseudoProbeForProfiling=*/false, CodeGenOpts.AtomicProfileUpdate);
   else if (CodeGenOpts.hasProfileIRUse()) {
 // -fprofile-use.
@@ -779,28 +780,32 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
 PGOOpt = PGOOptions(
 CodeGenOpts.ProfileInstrumentUsePath, "",
 CodeGenOpts.ProfileRemappingFile, CodeGenOpts.MemoryProfileUsePath, 
VFS,
-PGOOptions::IRUse, CSAction, CodeGenOpts.DebugInfoForProfiling);
+PGOOptions::IRUse, CSAction, PGOOptions::ColdFuncAttr::None,
+CodeGenOpts.DebugInfoForProfiling);
   } else if (!CodeGenOpts.SampleProfileFile.empty())
 // -fprofile-sample-use
 PGOOpt = PGOOptions(
 CodeGenOpts.SampleProfileFile, "", CodeGenOpts.ProfileRemappingFile,
 CodeGenOpts.MemoryProfileUsePath, VFS, PGOOptions::SampleUse,
-PGOOptions::NoCSAction, CodeGenOpts.DebugInfoForProfiling,
-CodeGenOpts.PseudoProbeForProfiling);
+PGOOptions::NoCSAction, PGOOptions::ColdFuncAttr::None,
+CodeGenOpts.DebugInfoForProfiling, 
CodeGenOpts.PseudoProbeForProfiling);
   else if (!CodeGenOpts.MemoryProfileUsePath.empty())
 // -fmemory-profile-use (without any of the above options)
 PGOOpt = PGOOptions("", "", "", CodeGenOpts.MemoryProfileUsePath, VFS,
 PGOOptions::NoAction, PGOOptions::NoCSAction,
+PGOOptions::ColdFuncAttr::None,
 CodeGenOpts.DebugInfoForProfiling);
   else if (CodeGenOpts.PseudoProbeForProfiling)
 // -fpseudo-probe-for-profiling
 PGOOpt = PGOOptions("", "", "", /*MemoryProfile=*/"", nullptr,
 PGOOptions::NoAction, PGOOptions::NoCSAction,
+PGOOptions::ColdFuncAttr::None,
 CodeGenOpts.DebugInfoForProfiling, true);
   else if (CodeGenOpts.DebugInfoForProfiling)
 // -fdebug-info-for-profiling
 PGOOpt = PGOOptions("", "", "", /*MemoryProfile=*/"", nullptr,
-PGOOptions::NoAction, PGOOptions::NoCSAction, true);
+PGOOptions::NoAction, PGOOptions::NoCSAction,
+PGOOptions::ColdFuncAttr::None, true);
 
   // Check to see if we want to generate a CS profile.
   if (CodeGenOpts.hasProfileCSIRInstr()) {
@@ -823,7 +828,8 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
  ? getDefaultProfileGenName()
  : CodeGenOpts.InstrProfileOutput,
  "", /*MemoryProfile=*/"", nullptr, PGOOptions::NoAction,
- PGOOptions::CSIRInstr, CodeGenOpts.DebugInfoForProfiling);
+ PGOOptions::CSIRInstr, PGOOptions::ColdFuncAttr::None,
+ CodeGenOpts.DebugInfoForProfiling);
   }
   if (TM)
 TM->setPGOOption(PGOOpt);
diff --git a

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-13 Thread Arthur Eubanks via cfe-commits 

aeubanks wrote:

still need to do measurements with this new approach, but lmk if this makes 
sense

https://github.com/llvm/llvm-project/pull/69030
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [PGO] Add ability to mark cold functions as optsize/minsize/optnone (PR #69030)

2023-10-13 Thread Arthur Eubanks via cfe-commits 

https://github.com/aeubanks created 
https://github.com/llvm/llvm-project/pull/69030

The performance of cold functions shouldn't matter too much, so if we care 
about binary sizes, add an option to mark cold functions as optsize/minsize for 
binary size, or optnone for compile times [1]. Clang patch will be in a future 
patch

Initial version: https://reviews.llvm.org/D149800

[1] 
https://discourse.llvm.org/t/rfc-new-feature-proposal-de-optimizing-cold-functions-using-pgo-info/56388


>From 81083bddc68864a96f265910019b519d28601e60 Mon Sep 17 00:00:00 2001
From: Arthur Eubanks 
Date: Fri, 13 Oct 2023 14:40:28 -0700
Subject: [PATCH] [PGO] Add ability to mark cold functions as
 optsize/minsize/optnone

The performance of cold functions shouldn't matter too much, so if we care 
about binary sizes, add an option to mark cold functions as optsize/minsize for 
binary size, or optnone for compile times [1]. Clang patch will be in a future 
patch

Initial version: https://reviews.llvm.org/D149800

[1] 
https://discourse.llvm.org/t/rfc-new-feature-proposal-de-optimizing-cold-functions-using-pgo-info/56388
---
 clang/lib/CodeGen/BackendUtil.cpp | 18 ++--
 llvm/include/llvm/Support/PGOOptions.h|  3 +
 .../Instrumentation/MarkColdFunctions.h   | 28 ++
 llvm/lib/LTO/LTOBackend.cpp   | 12 ++-
 llvm/lib/Passes/PassBuilder.cpp   |  3 +-
 llvm/lib/Passes/PassBuilderPipelines.cpp  | 12 +++
 llvm/lib/Passes/PassRegistry.def  |  1 +
 llvm/lib/Support/PGOOptions.cpp   |  7 +-
 .../Transforms/Instrumentation/CMakeLists.txt |  1 +
 .../Instrumentation/MarkColdFunctions.cpp | 65 +
 .../Transforms/MarkColdFunctions/basic.ll | 97 +++
 llvm/tools/opt/NewPMDriver.cpp| 24 -
 .../lib/Transforms/Instrumentation/BUILD.gn   |  1 +
 13 files changed, 253 insertions(+), 19 deletions(-)
 create mode 100644 
llvm/include/llvm/Transforms/Instrumentation/MarkColdFunctions.h
 create mode 100644 llvm/lib/Transforms/Instrumentation/MarkColdFunctions.cpp
 create mode 100644 llvm/test/Transforms/MarkColdFunctions/basic.ll

diff --git a/clang/lib/CodeGen/BackendUtil.cpp 
b/clang/lib/CodeGen/BackendUtil.cpp
index d066819871dfde3..f0c9b9d4daf08b5 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -770,7 +770,8 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
 CodeGenOpts.InstrProfileOutput.empty() ? getDefaultProfileGenName()
: 
CodeGenOpts.InstrProfileOutput,
 "", "", CodeGenOpts.MemoryProfileUsePath, nullptr, PGOOptions::IRInstr,
-PGOOptions::NoCSAction, CodeGenOpts.DebugInfoForProfiling,
+PGOOptions::NoCSAction, PGOOptions::ColdFuncAttr::None,
+CodeGenOpts.DebugInfoForProfiling,
 /*PseudoProbeForProfiling=*/false, CodeGenOpts.AtomicProfileUpdate);
   else if (CodeGenOpts.hasProfileIRUse()) {
 // -fprofile-use.
@@ -779,28 +780,32 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
 PGOOpt = PGOOptions(
 CodeGenOpts.ProfileInstrumentUsePath, "",
 CodeGenOpts.ProfileRemappingFile, CodeGenOpts.MemoryProfileUsePath, 
VFS,
-PGOOptions::IRUse, CSAction, CodeGenOpts.DebugInfoForProfiling);
+PGOOptions::IRUse, CSAction, PGOOptions::ColdFuncAttr::None,
+CodeGenOpts.DebugInfoForProfiling);
   } else if (!CodeGenOpts.SampleProfileFile.empty())
 // -fprofile-sample-use
 PGOOpt = PGOOptions(
 CodeGenOpts.SampleProfileFile, "", CodeGenOpts.ProfileRemappingFile,
 CodeGenOpts.MemoryProfileUsePath, VFS, PGOOptions::SampleUse,
-PGOOptions::NoCSAction, CodeGenOpts.DebugInfoForProfiling,
-CodeGenOpts.PseudoProbeForProfiling);
+PGOOptions::NoCSAction, PGOOptions::ColdFuncAttr::None,
+CodeGenOpts.DebugInfoForProfiling, 
CodeGenOpts.PseudoProbeForProfiling);
   else if (!CodeGenOpts.MemoryProfileUsePath.empty())
 // -fmemory-profile-use (without any of the above options)
 PGOOpt = PGOOptions("", "", "", CodeGenOpts.MemoryProfileUsePath, VFS,
 PGOOptions::NoAction, PGOOptions::NoCSAction,
+PGOOptions::ColdFuncAttr::None,
 CodeGenOpts.DebugInfoForProfiling);
   else if (CodeGenOpts.PseudoProbeForProfiling)
 // -fpseudo-probe-for-profiling
 PGOOpt = PGOOptions("", "", "", /*MemoryProfile=*/"", nullptr,
 PGOOptions::NoAction, PGOOptions::NoCSAction,
+PGOOptions::ColdFuncAttr::None,
 CodeGenOpts.DebugInfoForProfiling, true);
   else if (CodeGenOpts.DebugInfoForProfiling)
 // -fdebug-info-for-profiling
 PGOOpt = PGOOptions("", "", "", /*MemoryProfile=*/"", nullptr,
-PGOOptions::NoAction, PGOOptions::NoCSAction, true);
+PGOOptions::NoAction, PGOOptions::NoCSAction,
+