[PATCH] D132186: Clang: Add a new flag Wmisnoinline for printing hot noinline functions

[Archit Saxena via Phabricator via cfe-commits]

iamarchit123 added a comment.

Hi  @modimo @paulkirth any good open source benchmarks where I also can test 
this? The problem is finding some standard benchmarks which are profiled as 
well as have standard performance measuring metric to measure improvement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132186

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

[PATCH] D132186: Clang add front flags for Wmisnoinline

[Archit Saxena via Phabricator via cfe-commits]

iamarchit123 created this revision.
Herald added subscribers: mtrofin, ormris, ChuanqiXu, haicheng, hiraditya.
Herald added a project: All.
iamarchit123 requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Test Plan:


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132186

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/Analysis/InlineCost.h
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/LLVMContext.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/Analysis/InlineAdvisor.cpp
  llvm/lib/Analysis/InlineCost.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/LLVMContext.cpp
  llvm/lib/IR/LLVMContextImpl.h

Index: llvm/lib/IR/LLVMContextImpl.h
===
--- llvm/lib/IR/LLVMContextImpl.h
+++ llvm/lib/IR/LLVMContextImpl.h
@@ -1391,6 +1391,11 @@
   Optional DiagnosticsMisExpectTolerance = 0;
   bool MisExpectWarningRequested = false;
 
+  /// The percentile of hotness of a function
+  //  when emiting MisNoInline diagnostics
+  Optional DiagnosticsMisNoInlinePercentile = 99;
+  bool MisNoInlineWarningRequested = false;
+
   /// The specialized remark streamer used by LLVM's OptimizationRemarkEmitter.
   std::unique_ptr LLVMRS;
 
Index: llvm/lib/IR/LLVMContext.cpp
===
--- llvm/lib/IR/LLVMContext.cpp
+++ llvm/lib/IR/LLVMContext.cpp
@@ -155,6 +155,19 @@
   return pImpl->DiagnosticsMisExpectTolerance.value_or(0);
 }
 
+void LLVMContext::setMisNoInlineWarningRequested(bool Requested) {
+  pImpl->MisNoInlineWarningRequested = Requested;
+}
+bool LLVMContext::getMisNoInlineWarningRequested() const {
+  return pImpl->MisNoInlineWarningRequested;
+}
+void LLVMContext::setDiagnosticsMisNoInlinePercentileThreshold(
+Optional Count) {
+  pImpl->DiagnosticsMisNoInlinePercentile = Count;
+}
+uint64_t LLVMContext::getDiagnosticsMisNoInlinePercentileThreshold() const {
+  return pImpl->DiagnosticsMisNoInlinePercentile.value_or(0);
+}
 bool LLVMContext::isDiagnosticsHotnessThresholdSetFromPSI() const {
   return !pImpl->DiagnosticsHotnessThreshold.has_value();
 }
Index: llvm/lib/IR/DiagnosticInfo.cpp
===
--- llvm/lib/IR/DiagnosticInfo.cpp
+++ llvm/lib/IR/DiagnosticInfo.cpp
@@ -404,6 +404,19 @@
   DP << getLocationStr() << ": " << getMsg();
 }
 
+DiagnosticInfoMisNoInline::DiagnosticInfoMisNoInline(const Instruction *Inst,
+ const Function &Fn,
+ Twine &Msg)
+: DiagnosticInfoWithLocationBase(DK_MisNoInline, DS_Warning,
+ *Inst->getParent()->getParent(),
+ Inst->getDebugLoc()),
+  Fn(Fn), Msg(Msg) {}
+
+void DiagnosticInfoMisNoInline::print(DiagnosticPrinter &DP) const {
+  DP << "Module"
+ << ": " << getFunction().getParent()->getName() << " " << getMsg();
+}
+
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
 
Index: llvm/lib/Analysis/InlineCost.cpp
===
--- llvm/lib/Analysis/InlineCost.cpp
+++ llvm/lib/Analysis/InlineCost.cpp
@@ -2919,33 +2919,29 @@
 return InlineResult::failure("interposable");
 
   // Don't inline functions marked noinline.
-  if (Callee->hasFnAttribute(Attribute::NoInline))
-return InlineResult::failure("noinline function attribute");
+  if (Callee->hasFnAttribute(Attribute::NoInline)) {
+auto IR = InlineResult::failure("noinline function attribute");
+IR.setIsNoInline();
+return IR;
+  }
 
   // Don't inline call sites marked noinline.
-  if (Call.isNoInline())
-return InlineResult::failure("noinline call site attribute");
+  if (Call.isNoInline()) {
+auto IR = InlineResult::failure("noinline call site attribute");
+IR.setIsNoInline();
+return IR;
+  }
 
   return None;
 }
 
-InlineCost llvm::getInlineCost(
+InlineCost llvm::isInlineViableFromCostAnalysis(
 CallBase &Call, Function *Callee, const InlineParams &Params,
 TargetTransformInfo &CalleeTTI,
 function_ref GetAssumptionCache,
 function_ref GetTLI,
 function_ref GetBFI,
 ProfileSummaryInfo *PSI, OptimizationRemarkEmitter *ORE) {
-
-  auto UserDecision =
-  llvm::getAttributeBasedInliningDecision(Call, Callee, CalleeTTI, GetTLI);
-
-  if (UserDecision) {
-if (UserDecision->isSuccess())
-  return llv

[PATCH] D132188: Add documentation and test cases for Wmisnoinline

[Archit Saxena via Phabricator via cfe-commits]

iamarchit123 created this revision.
Herald added a subscriber: wenlei.
Herald added a project: All.
iamarchit123 requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Test Plan:


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132188

Files:
  clang/docs/MisNoInline.rst
  clang/docs/ReleaseNotes.rst
  clang/test/Misc/Inputs/MisNoInline.proftext
  clang/test/Misc/MisNoInline.cpp
  clang/test/Misc/MisNoInline_LowThreshold.cpp
  clang/test/Misc/MisNoInline_PragmaIgnore.cpp
  llvm/docs/MisNoInline.rst
  llvm/test/Transforms/PGOProfile/Inputs/MisNoInline.proftext
  llvm/test/Transforms/PGOProfile/MisNoInline.ll

Index: llvm/test/Transforms/PGOProfile/MisNoInline.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/MisNoInline.ll
@@ -0,0 +1,216 @@
+; RUN: opt < %s -passes=sample-profile,inline -sample-profile-file=%S/Inputs/MisNoInline.proftext -pgo-warn-misnoinline -S  2>&1 | FileCheck %s -check-prefix=WARNING
+; RUN: opt < %s -passes=sample-profile,inline -sample-profile-file=%S/Inputs/MisNoInline.proftext -pgo-warn-misnoinline -misnoinline-percent=90 -S  2>&1 | FileCheck %s -check-prefix=WARNING_LOW
+; RUN: opt < %s -passes=sample-profile,inline -sample-profile-file=%S/Inputs/MisNoInline.proftext -pass-remarks=misnoinline -S 2>&1 | FileCheck %s --check-prefix=REMARK
+
+; WARNING-DAG: warning: Module: {{.*}} Marking _Z4foo1ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+; WARNING-DAG: warning: Module: {{.*}} Marking _Z4foo2ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+
+; WARNING_LOW-DAG: warning: Module: {{.*}} Marking _Z4foo1ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+; WARNING_LOW-NOT: warning: Module: {{.*}} Marking _Z4foo2ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+
+; REMARK-NOT: warning: Module: {{.*}} Marking _Z4foo1ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+; REMARK-NOT: warning: Module: {{.*}} Marking _Z4foo2ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+; REMARK-DAG: remark: MisNoInline.cpp:28:8: Marking _Z4foo1ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+; REMARK-DAG: remark: MisNoInline.cpp:29:8: Marking _Z4foo2ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+
+; The source code used for the LLVM IR that follows.
+; void printf();
+; __attribute__((noinline)) long foo1(int x, int y) {
+;   while (x != y) {
+; printf();
+; y++;
+;   }
+;   return y;
+; }
+; __attribute__((noinline)) long foo2(int x, int y) {
+;   while (x != y) {
+; printf();
+; y++;
+;   }
+;   return y;
+; }
+; int main() {
+;   int x = 5678;
+;   int y = 1234;
+;   x += foo1(x, y);
+;   x += foo2(x, y);
+;
+;   return x;
+; }
+
+; Function Attrs: mustprogress noinline optnone uwtable
+define dso_local noundef i64 @_Z4foo1ii(i32 noundef %x, i32 noundef %y) #0 !dbg !8 {
+entry:
+  %x.addr = alloca i32, align 4
+  %y.addr = alloca i32, align 4
+  store i32 %x, ptr %x.addr, align 4
+  call void @llvm.dbg.declare(metadata ptr %x.addr, metadata !14, metadata !DIExpression()), !dbg !15
+  store i32 %y, ptr %y.addr, align 4
+  call void @llvm.dbg.declare(metadata ptr %y.addr, metadata !16, metadata !DIExpression()), !dbg !17
+  br label %while.cond, !dbg !18
+
+while.cond:   ; preds = %while.body, %entry
+  %0 = load i32, ptr %x.addr, align 4, !dbg !19
+  %1 = load i32, ptr %y.addr, align 4, !dbg !21
+  %cmp = icmp ne i32 %0, %1, !dbg !22
+  br i1 %cmp, label %while.body, label %while.end, !dbg !23
+
+while.body:   ; preds = %while.cond
+  call void @_Z6printfv(), !dbg !24
+  %2 = load i32, ptr %y.addr, align 4, !dbg !26
+  %inc = add nsw i32 %2, 1, !dbg !26
+  store i32 %inc, ptr %y.addr, align 4, !dbg !26
+  br label %while.cond, !dbg !27, !llvm.loop !29
+
+while.end:; preds = %while.cond
+  %3 = load i32, ptr %y.addr, align 4, !dbg !32
+  %conv = sext i32 %3 to i64, !dbg !32
+  ret i64 %conv, !dbg !33
+}
+
+; Function Attrs: nocallback nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+declare dso_local void @_Z6printfv() #2
+
+; Function Attrs: mustprogress noinline optnone uwtable
+define dso_local noundef i64 @_Z4foo2ii(i32 noundef %x, i32 noundef %y) #0 !dbg !34 {
+entry:
+  %x.addr = alloca i32, align 4
+  %y.addr = alloca i32, align 4
+  store i32 %x, ptr %x.addr, align 4
+  call void @llvm.dbg.declare(metadata ptr %x.addr, metadata !35, metadata !DIExpression()), !dbg !36
+  store i32 %y, ptr %y.addr, align 4

[PATCH] D132186: Clang add front flags for Wmisnoinline

[Archit Saxena via Phabricator via cfe-commits]

iamarchit123 updated this revision to Diff 453828.
iamarchit123 added a comment.
Herald added a subscriber: wenlei.

update commit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132186

Files:
  clang/docs/MisNoInline.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Misc/Inputs/MisNoInline.proftext
  clang/test/Misc/MisNoInline.cpp
  clang/test/Misc/MisNoInline_LowThreshold.cpp
  clang/test/Misc/MisNoInline_PragmaIgnore.cpp
  llvm/docs/MisNoInline.rst
  llvm/include/llvm/Analysis/InlineCost.h
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/LLVMContext.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/Analysis/InlineAdvisor.cpp
  llvm/lib/Analysis/InlineCost.cpp
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/LLVMContext.cpp
  llvm/lib/IR/LLVMContextImpl.h
  llvm/test/Transforms/PGOProfile/Inputs/MisNoInline.proftext
  llvm/test/Transforms/PGOProfile/MisNoInline.ll

Index: llvm/test/Transforms/PGOProfile/MisNoInline.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/MisNoInline.ll
@@ -0,0 +1,216 @@
+; RUN: opt < %s -passes=sample-profile,inline -sample-profile-file=%S/Inputs/MisNoInline.proftext -pgo-warn-misnoinline -S  2>&1 | FileCheck %s -check-prefix=WARNING
+; RUN: opt < %s -passes=sample-profile,inline -sample-profile-file=%S/Inputs/MisNoInline.proftext -pgo-warn-misnoinline -misnoinline-percent=90 -S  2>&1 | FileCheck %s -check-prefix=WARNING_LOW
+; RUN: opt < %s -passes=sample-profile,inline -sample-profile-file=%S/Inputs/MisNoInline.proftext -pass-remarks=misnoinline -S 2>&1 | FileCheck %s --check-prefix=REMARK
+
+; WARNING-DAG: warning: Module: {{.*}} Marking _Z4foo1ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+; WARNING-DAG: warning: Module: {{.*}} Marking _Z4foo2ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+
+; WARNING_LOW-DAG: warning: Module: {{.*}} Marking _Z4foo1ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+; WARNING_LOW-NOT: warning: Module: {{.*}} Marking _Z4foo2ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+
+; REMARK-NOT: warning: Module: {{.*}} Marking _Z4foo1ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+; REMARK-NOT: warning: Module: {{.*}} Marking _Z4foo2ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+; REMARK-DAG: remark: MisNoInline.cpp:28:8: Marking _Z4foo1ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+; REMARK-DAG: remark: MisNoInline.cpp:29:8: Marking _Z4foo2ii noinline while calling in main may hurt performance as per inline cost/hotness analysis
+
+; The source code used for the LLVM IR that follows.
+; void printf();
+; __attribute__((noinline)) long foo1(int x, int y) {
+;   while (x != y) {
+; printf();
+; y++;
+;   }
+;   return y;
+; }
+; __attribute__((noinline)) long foo2(int x, int y) {
+;   while (x != y) {
+; printf();
+; y++;
+;   }
+;   return y;
+; }
+; int main() {
+;   int x = 5678;
+;   int y = 1234;
+;   x += foo1(x, y);
+;   x += foo2(x, y);
+;
+;   return x;
+; }
+
+; Function Attrs: mustprogress noinline optnone uwtable
+define dso_local noundef i64 @_Z4foo1ii(i32 noundef %x, i32 noundef %y) #0 !dbg !8 {
+entry:
+  %x.addr = alloca i32, align 4
+  %y.addr = alloca i32, align 4
+  store i32 %x, ptr %x.addr, align 4
+  call void @llvm.dbg.declare(metadata ptr %x.addr, metadata !14, metadata !DIExpression()), !dbg !15
+  store i32 %y, ptr %y.addr, align 4
+  call void @llvm.dbg.declare(metadata ptr %y.addr, metadata !16, metadata !DIExpression()), !dbg !17
+  br label %while.cond, !dbg !18
+
+while.cond:   ; preds = %while.body, %entry
+  %0 = load i32, ptr %x.addr, align 4, !dbg !19
+  %1 = load i32, ptr %y.addr, align 4, !dbg !21
+  %cmp = icmp ne i32 %0, %1, !dbg !22
+  br i1 %cmp, label %while.body, label %while.end, !dbg !23
+
+while.body:   ; preds = %while.cond
+  call void @_Z6printfv(), !dbg !24
+  %2 = load i32, ptr %y.addr, align 4, !dbg !26
+  %inc = add nsw i32 %2, 1, !dbg !26
+  store i32 %inc, ptr %y.addr, align 4, !dbg !26
+  br label %while.cond, !dbg !27, !llvm.loop !29
+
+while.end:; preds = %while.cond
+  %3 = load i32, ptr %y.add

[PATCH] D132186: Clang: Add a new flag Wmisnoinline for printing hot noinline functions

[Archit Saxena via Phabricator via cfe-commits]

iamarchit123 added a comment.

In D132186#3741210 , @paulkirth wrote:

> Hi, thanks for taking a look at this. Before we start an in-depth review, can 
> you describe the deficiencies w/ the existing diagnostics, and why they don't 
> meet your needs?
>
> Primarily, I'm a little skeptical if taking the same approach as MisExpect is 
> the correct approach.
>
> Unlike `llvm.expect`, the `noinline` attribute is often used for correctness. 
> I'm not sure it makes sense to warn about it in the same way as performance 
> optimization. My experience may differ from the code bases you work in, but I 
> cannot recall seeing a function annotated `noinline` for due to any kind of 
> performance reason. The one exception I can think of being code marked as 
> `cold` for outlining purposes, but those are usually inferred from profiles 
> or are added due to other annotations. Do people do this for better i-cache 
> performance or something?

I was under the impression that it was used for performance reasons and hot 
functions should not ideally be marked noinline. @modimo could you also pitch 
in on this? If not function entry count what other things could possibly 
indicate a false usage of noinline(if not the hotness)

> MisExpect diagnostics need to run a specific points during compilation to 
> check the weights added by the `llvm.expect` intrinsic against the profile, 
> so it can't be a separate pass, since e.g., LowerExpectIntrinsicPass and the 
> PGO passes for instrumentation/sampling replace/remove that information. From 
> what I can see this could be its own analysis pass, since you at most need to 
> consult the function entry count.

This can be a separate pass but even I am using Cost Analysis to check noinline 
functions cost apart from function entry count, so it felt natural to integrate 
the noinline warning into inline cost analysis.

> Optimization remarks already exist for missed inlining opportunities. I'm 
> unsure of the value in using a warning over the existing diagnostic in this 
> case. In the case of `llvm.expect` intrinsics, it may be the result of an 
> incorrect annotation, or a mis-annotated branch (i.e., marking a branch w/ 
> `LIKELY` instead of `UNLIKELY`). In these cases, we'd like to signal to users 
> a problem w/ the source code/annotation. I'm not sure that the same is true 
> for `noinline` attributes. Is this something you want to use to fail builds? 
> That was something we wanted to achieve for Fuchsia's CI, which is why 
> `-Wmisexpect` exists as more than an optimization remark.

I think there is no Optimization remark emission for missed inlining 
opportunities for noinline functions as cost analysis is skipped for functions 
marked with noinline attribute. No we don't want any builds to fail with this. 
Our only aim to make this change is to emit a warning for functions which a 
user may have accidentally marked noinline but removing noinline attribute may 
give some performance benefits.

@modimo feel free to pitch in as well on this about concerns raised by 
@paulkirth and any changes I could make on this based on the comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132186

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

[PATCH] D132186: Clang: Add a new flag Wmisnoinline for printing hot noinline functions

[Archit Saxena via Phabricator via cfe-commits]

iamarchit123 added a comment.

@paulkirth  this change was done under the intuition that marking hot functions 
noinline may hurt performance. This change till now hasn't been tested for 
performance improvements and so the points that you raise that functions marked 
noinline are marked not for hotness/performance but rather correctness is 
something that I was unaware of. Since I don't have access to big services 
currently and can't test this change, so I may be unable to defend this change. 
@modimo feel free if you think folks can come back and test this change for 
effectiveness or completely redo this if the above change is still insufficient.

I agree until it's shown/proven that there is a serious performance win this 
change may not be useful. Thanks for the review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132186

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