[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin 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 rG5fe10263ab39: [llvm][inliner] Reuse the inliner pass to 
implement 'always inliner' (authored by mtrofin).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

Files:
  clang/test/CodeGen/thinlto-distributed-newpm.ll
  clang/test/Frontend/optimization-remark-line-directive.c
  clang/test/Frontend/optimization-remark-new-pm.c
  clang/test/Frontend/optimization-remark-with-hotness-new-pm.c
  clang/test/Frontend/optimization-remark.c
  llvm/include/llvm/Analysis/InlineAdvisor.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Analysis/InlineAdvisor.cpp
  llvm/lib/Analysis/MLInlineAdvisor.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/Inliner.cpp
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-lto-defaults.ll
  llvm/test/Other/new-pm-module-inliner-wrapper.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
  llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
  llvm/test/Transforms/Inline/ML/bounds-checks.ll
  llvm/test/Transforms/Inline/inline_stats.ll
  llvm/test/Transforms/Inline/pr46945.ll

Index: llvm/test/Transforms/Inline/pr46945.ll
===
--- llvm/test/Transforms/Inline/pr46945.ll
+++ llvm/test/Transforms/Inline/pr46945.ll
@@ -1,12 +1,13 @@
-; RUN: opt %s -o - -S -passes=inliner-wrapper | FileCheck %s
+; RUN: opt %s -o - -S -passes=always-inliner-wrapper | FileCheck %s
+; RUN: opt %s -o - -S -passes='default' | FileCheck %s
+; RUN: opt %s -o - -S -passes=inliner-wrapper | FileCheck %s -check-prefix=BASELINE
 
-; CHECK-NOT: call void @b()
-define void @a() {
-entry:
-  call void @b()
-  ret void
-}
+; In the baseline case, a will be first inlined into b, which makes c recursive,
+; and, thus, un-inlinable. We need a baseline case to make sure intra-SCC order
+; is as expected: b first, then a.
 
+; BASELINE: call void @b()
+; CHECK-NOT: call void @b()
 define void @b() alwaysinline {
 entry:
   br label %for.cond
@@ -16,3 +17,8 @@
   br label %for.cond
 }
 
+define void @a() {
+entry:
+  call void @b()
+  ret void
+}
Index: llvm/test/Transforms/Inline/inline_stats.ll
===
--- llvm/test/Transforms/Inline/inline_stats.ll
+++ llvm/test/Transforms/Inline/inline_stats.ll
@@ -6,8 +6,11 @@
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
 
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
+
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
 
 ; CHECK: --- Dumping inliner stats for [] ---
 ; CHECK-BASIC-NOT: -- List of inlined functions:
Index: llvm/test/Transforms/Inline/ML/bounds-checks.ll
===
--- llvm/test/Transforms/Inline/ML/bounds-checks.ll
+++ llvm/test/Transforms/Inline/ML/bounds-checks.ll
@@ -4,7 +4,7 @@
 ; factor, we don't inline anymore.
 ; REQUIRES: have_tf_aot
 ; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=10.0 -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=NOBOUNDS
-; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=1.0 -disable-always-inliner-in-module-wrapper -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=BOUNDS
+; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=1.0 -S < %s 2>&1 | FileCheck %s --check-

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin added inline comments.



Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:27
 
 /// There are 3 scenarios we can use the InlineAdvisor:
 /// - Default - use manual heuristics.

aeubanks wrote:
> aeubanks wrote:
> > 4
> ping
sorry - done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin updated this revision to Diff 308441.
mtrofin marked 5 inline comments as done.
mtrofin added a comment.

fixes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

Files:
  clang/test/CodeGen/thinlto-distributed-newpm.ll
  clang/test/Frontend/optimization-remark-line-directive.c
  clang/test/Frontend/optimization-remark-new-pm.c
  clang/test/Frontend/optimization-remark-with-hotness-new-pm.c
  clang/test/Frontend/optimization-remark.c
  llvm/include/llvm/Analysis/InlineAdvisor.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Analysis/InlineAdvisor.cpp
  llvm/lib/Analysis/MLInlineAdvisor.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/Inliner.cpp
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-lto-defaults.ll
  llvm/test/Other/new-pm-module-inliner-wrapper.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
  llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
  llvm/test/Transforms/Inline/ML/bounds-checks.ll
  llvm/test/Transforms/Inline/inline_stats.ll
  llvm/test/Transforms/Inline/pr46945.ll

Index: llvm/test/Transforms/Inline/pr46945.ll
===
--- llvm/test/Transforms/Inline/pr46945.ll
+++ llvm/test/Transforms/Inline/pr46945.ll
@@ -1,12 +1,13 @@
-; RUN: opt %s -o - -S -passes=inliner-wrapper | FileCheck %s
+; RUN: opt %s -o - -S -passes=always-inliner-wrapper | FileCheck %s
+; RUN: opt %s -o - -S -passes='default' | FileCheck %s
+; RUN: opt %s -o - -S -passes=inliner-wrapper | FileCheck %s -check-prefix=BASELINE
 
-; CHECK-NOT: call void @b()
-define void @a() {
-entry:
-  call void @b()
-  ret void
-}
+; In the baseline case, a will be first inlined into b, which makes c recursive,
+; and, thus, un-inlinable. We need a baseline case to make sure intra-SCC order
+; is as expected: b first, then a.
 
+; BASELINE: call void @b()
+; CHECK-NOT: call void @b()
 define void @b() alwaysinline {
 entry:
   br label %for.cond
@@ -16,3 +17,8 @@
   br label %for.cond
 }
 
+define void @a() {
+entry:
+  call void @b()
+  ret void
+}
Index: llvm/test/Transforms/Inline/inline_stats.ll
===
--- llvm/test/Transforms/Inline/inline_stats.ll
+++ llvm/test/Transforms/Inline/inline_stats.ll
@@ -6,8 +6,11 @@
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
 
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
+
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
 
 ; CHECK: --- Dumping inliner stats for [] ---
 ; CHECK-BASIC-NOT: -- List of inlined functions:
Index: llvm/test/Transforms/Inline/ML/bounds-checks.ll
===
--- llvm/test/Transforms/Inline/ML/bounds-checks.ll
+++ llvm/test/Transforms/Inline/ML/bounds-checks.ll
@@ -4,7 +4,7 @@
 ; factor, we don't inline anymore.
 ; REQUIRES: have_tf_aot
 ; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=10.0 -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=NOBOUNDS
-; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=1.0 -disable-always-inliner-in-module-wrapper -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=BOUNDS
+; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=1.0 -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=BOUNDS
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 tar

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks accepted this revision.
aeubanks added a comment.
This revision is now accepted and ready to land.

aside from some nits, lgtm
thanks for doing this!




Comment at: clang/test/Frontend/optimization-remark-line-directive.c:5
 
-// RUN: %clang_cc1 %s -Rpass=inline -debug-info-kind=line-tables-only 
-emit-llvm-only -verify -fno-experimental-new-pass-manager
+// RUN: %clang_cc1 %s -Rpass=inline -debug-info-kind=line-tables-only 
-emit-llvm-only -verify -fno-experimental-new-pass-manager -mllvm 
-mandatory-inlining-first=0
 

the change on this line shouldn't be necessary, this is a legacy PM RUN line



Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:27
 
 /// There are 3 scenarios we can use the InlineAdvisor:
 /// - Default - use manual heuristics.

aeubanks wrote:
> 4
ping



Comment at: llvm/test/Transforms/Inline/pr46945.ll:1-2
-; RUN: opt %s -o - -S -passes=inliner-wrapper | FileCheck %s
+; RUN: opt %s -o - -S -passes=always-inliner-wrapper | FileCheck %s
+; RUN: opt %s -o - -S -passes=inliner-wrapper | FileCheck %s 
-check-prefix=BASELINE
 

maybe we should have a RUN line with `-passes='default'` to make sure the 
whole thing works


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin updated this revision to Diff 308422.
mtrofin added a comment.

Fixed the LTO case.

Also fixed the p46945 test, which, post - D90566 
, was passing without the need of a 
preliminary always-inlier pass.
The reason is that the order of the traversal of the functions in a SCC 
changed. The test requies that the 'alwaysinline'
function be processed first (to render it recursive and, thus, uninlinable).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

Files:
  clang/test/CodeGen/thinlto-distributed-newpm.ll
  clang/test/Frontend/optimization-remark-line-directive.c
  clang/test/Frontend/optimization-remark-new-pm.c
  clang/test/Frontend/optimization-remark-with-hotness-new-pm.c
  clang/test/Frontend/optimization-remark.c
  llvm/include/llvm/Analysis/InlineAdvisor.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Analysis/InlineAdvisor.cpp
  llvm/lib/Analysis/MLInlineAdvisor.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/Inliner.cpp
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-lto-defaults.ll
  llvm/test/Other/new-pm-module-inliner-wrapper.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
  llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
  llvm/test/Transforms/Inline/ML/bounds-checks.ll
  llvm/test/Transforms/Inline/inline_stats.ll
  llvm/test/Transforms/Inline/pr46945.ll

Index: llvm/test/Transforms/Inline/pr46945.ll
===
--- llvm/test/Transforms/Inline/pr46945.ll
+++ llvm/test/Transforms/Inline/pr46945.ll
@@ -1,12 +1,12 @@
-; RUN: opt %s -o - -S -passes=inliner-wrapper | FileCheck %s
+; RUN: opt %s -o - -S -passes=always-inliner-wrapper | FileCheck %s
+; RUN: opt %s -o - -S -passes=inliner-wrapper | FileCheck %s -check-prefix=BASELINE
 
-; CHECK-NOT: call void @b()
-define void @a() {
-entry:
-  call void @b()
-  ret void
-}
+; In the baseline case, a will be first inlined into b, which makes c recursive,
+; and, thus, un-inlinable. We need a baseline case to make sure intra-SCC order
+; is as expected: b first, then a.
 
+; BASELINE: call void @b()
+; CHECK-NOT: call void @b()
 define void @b() alwaysinline {
 entry:
   br label %for.cond
@@ -16,3 +16,8 @@
   br label %for.cond
 }
 
+define void @a() {
+entry:
+  call void @b()
+  ret void
+}
Index: llvm/test/Transforms/Inline/inline_stats.ll
===
--- llvm/test/Transforms/Inline/inline_stats.ll
+++ llvm/test/Transforms/Inline/inline_stats.ll
@@ -6,8 +6,11 @@
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
 
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
+
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
 
 ; CHECK: --- Dumping inliner stats for [] ---
 ; CHECK-BASIC-NOT: -- List of inlined functions:
Index: llvm/test/Transforms/Inline/ML/bounds-checks.ll
===
--- llvm/test/Transforms/Inline/ML/bounds-checks.ll
+++ llvm/test/Transforms/Inline/ML/bounds-checks.ll
@@ -4,7 +4,7 @@
 ; factor, we don't inline anymore.
 ; REQUIRES: have_tf_aot
 ; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=10.0 -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=NOBOUNDS
-; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=1.0 -disable-always-inliner-in-module-wrapper -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=BOUNDS
+; RUN: opt

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

From a ThinLTO perspective, no specific concerns as the 
buildModuleSimplificationPipeline is invoked in both the pre and post LTO link 
pipelines, so they both get an equivalent change. But there is an issue for 
regular LTO, noted below.




Comment at: llvm/test/Other/new-pm-lto-defaults.ll:70
 ; CHECK-O2-NEXT: Starting llvm::Module pass manager run.
-; CHECK-O2-NEXT: Running pass: AlwaysInlinerPass
 ; CHECK-O2-NEXT: Starting CGSCC pass manager run.

Note there is no corresponding add of an additional InlinerPass like in the 
other files. The reason is that PassBuilder::buildLTODefaultPipeline doesn't 
invoke buildModuleSimplificationPipeline, or even buildInlinerPipeline (it has 
a separate pipeline setup for compile time reasons due to the monolithic nature 
of the post-LTO link compilation), but rather directly adds 
ModuleInlinerWrapperPass. So you'll want to add the additional 
ModuleInlinerWrapperPass invocation there as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin updated this revision to Diff 306593.
mtrofin added a comment.
Herald added subscribers: wenlei, steven_wu.

patched up tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

Files:
  clang/test/CodeGen/thinlto-distributed-newpm.ll
  clang/test/Frontend/optimization-remark-line-directive.c
  clang/test/Frontend/optimization-remark-new-pm.c
  clang/test/Frontend/optimization-remark-with-hotness-new-pm.c
  clang/test/Frontend/optimization-remark.c
  llvm/include/llvm/Analysis/InlineAdvisor.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Analysis/InlineAdvisor.cpp
  llvm/lib/Analysis/MLInlineAdvisor.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/Inliner.cpp
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-lto-defaults.ll
  llvm/test/Other/new-pm-module-inliner-wrapper.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
  llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
  llvm/test/Transforms/Inline/ML/bounds-checks.ll
  llvm/test/Transforms/Inline/inline_stats.ll

Index: llvm/test/Transforms/Inline/inline_stats.ll
===
--- llvm/test/Transforms/Inline/inline_stats.ll
+++ llvm/test/Transforms/Inline/inline_stats.ll
@@ -6,8 +6,11 @@
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
 
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
+
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
 
 ; CHECK: --- Dumping inliner stats for [] ---
 ; CHECK-BASIC-NOT: -- List of inlined functions:
Index: llvm/test/Transforms/Inline/ML/bounds-checks.ll
===
--- llvm/test/Transforms/Inline/ML/bounds-checks.ll
+++ llvm/test/Transforms/Inline/ML/bounds-checks.ll
@@ -4,7 +4,7 @@
 ; factor, we don't inline anymore.
 ; REQUIRES: have_tf_aot
 ; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=10.0 -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=NOBOUNDS
-; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=1.0 -disable-always-inliner-in-module-wrapper -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=BOUNDS
+; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=1.0 -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=BOUNDS
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-grtev4-linux-gnu"
Index: llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
===
--- llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
+++ llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
@@ -7,18 +7,18 @@
 ; REQUIRES: have_tf_api
 ;
 ; When the bounds are very wide ("no bounds"), all inlinings happen.
-; RUN: opt -passes=scc-oz-module-inliner -ml-inliner-ir2native-model=%S/../../../../unittests/Analysis/Inputs/ir2native_x86_64_model -ml-inliner-model-under-training=%S/../../../../lib/Analysis/models/inliner -training-log=- -enable-ml-inliner=development -ml-advisor-size-increase-threshold=10.0 -disable-always-inliner-in-module-wrapper -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=NOBOUNDS
+; RUN: opt -passes=scc-oz-module-inliner -ml-inliner-ir2native-model=%S/../../../../unittests/Analysis/Inputs/ir2native_x86_64_model -ml-inliner-model-under-training=%S/../../../../lib/Analysis/models/inliner -traini

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin added a comment.

In D91567#2403544 , @aeubanks wrote:

> In D91567#2403252 , @mtrofin wrote:
>
>> In D91567#2403236 , @aeubanks wrote:
>>
>>> One thing that would be nice would be to have both inliners in the same 
>>> CGSCC pass manager to avoid doing SCC construction twice, but that would 
>>> require some shuffling of module/cgscc passes in ModuleInlinerWrapperPass. 
>>> Maybe as a future cleanup.
>>
>> There's that benefit to simplifying the module with the always inliner 
>> before doing inlining "in earnest" I was pointing earlier at: for the ML 
>> policies work, we plan on capturing (sub)graph information. Using the same 
>> SCC would not help because the "higher" (callers) parts of the graph would 
>> have these mandatory inlinings not completed yet, and thus offer a less 
>> accurate picture of the problem space.
>
> Oh I see, caller information is useful.
>
> For compile times: 
> http://llvm-compile-time-tracker.com/?config=O3&stat=instructions&remote=aeubanks.
> The previous version of this patch (perf/npmalways) running a couple passes 
> has some small but measurable overhead on some benchmarks, 0.5%.
> The version of running everything (perf/npmalways2) hugely increases compile 
> times, almost by 50% in one case.

Thanks for doing this! Really good to have this data.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D91567#2403252 , @mtrofin wrote:

> In D91567#2403236 , @aeubanks wrote:
>
>> One thing that would be nice would be to have both inliners in the same 
>> CGSCC pass manager to avoid doing SCC construction twice, but that would 
>> require some shuffling of module/cgscc passes in ModuleInlinerWrapperPass. 
>> Maybe as a future cleanup.
>
> There's that benefit to simplifying the module with the always inliner before 
> doing inlining "in earnest" I was pointing earlier at: for the ML policies 
> work, we plan on capturing (sub)graph information. Using the same SCC would 
> not help because the "higher" (callers) parts of the graph would have these 
> mandatory inlinings not completed yet, and thus offer a less accurate picture 
> of the problem space.

Oh I see, caller information is useful.

For compile times: 
http://llvm-compile-time-tracker.com/?config=O3&stat=instructions&remote=aeubanks.
The previous version of this patch (perf/npmalways) running a couple passes has 
some small but measurable overhead on some benchmarks, 0.5%.
The version of running everything (perf/npmalways2) hugely increases compile 
times, almost by 50% in one case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin added a comment.

In D91567#2403236 , @aeubanks wrote:

> One thing that would be nice would be to have both inliners in the same CGSCC 
> pass manager to avoid doing SCC construction twice, but that would require 
> some shuffling of module/cgscc passes in ModuleInlinerWrapperPass. Maybe as a 
> future cleanup.

There's that benefit to simplifying the module with the always inliner before 
doing inlining "in earnest" I was pointing earlier at: for the ML policies 
work, we plan on capturing (sub)graph information. Using the same SCC would not 
help because the "higher" (callers) parts of the graph would have these 
mandatory inlinings not completed yet, and thus offer a less accurate picture 
of the problem space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

One thing that would be nice would be to have both inliners in the same CGSCC 
pass manager to avoid doing SCC construction twice, but that would require some 
shuffling of module/cgscc passes in ModuleInlinerWrapperPass. Maybe as a future 
cleanup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin added a comment.

In D91567#2403216 , @aeubanks wrote:

>>> I'll run this through llvm-compile-time-tracker to see what the compile 
>>> time implications are.
>>
>> You mean for the variant where we ran some of the function passes, or you'd 
>> try running all of them? Probably the latter would be quite interesting as a 
>> 'worst case'.
>
> I was trying the previous patch, but will also try running all function 
> passes, definitely would be interesting.

Awesome! Thanks!

If the rest of the (now NFC) patch seems reasonable, I'll go through those 
pesky pass manager tests to finish it up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin added a comment.

In D91567#2403173 , @aeubanks wrote:

> In D91567#2400699 , @mtrofin wrote:
>
>> In D91567#2400207 , @aeubanks wrote:
>>
>>> What about removing the existing AlwaysInlinerPass and replacing it with 
>>> this one? Or is that something you were planning to do in a follow-up 
>>> change?
>>
>> That's the plan, yes.
>>
 open the opportunity to help the full inliner by performing additional 
 function passes after the mandatory inlinings, but before the full inliner
>>>
>>> This change doesn't run the function simplification pipeline between the 
>>> mandatory and full inliner though, only
>>>
>>>   if (AttributorRun & AttributorRunOption::CGSCC)
>>> MainCGPipeline.addPass(AttributorCGSCCPass());
>>>   
>>>   if (PTO.Coroutines)
>>> MainCGPipeline.addPass(CoroSplitPass(Level != OptimizationLevel::O0));
>>>   
>>>   // Now deduce any function attributes based in the current code.
>>>   MainCGPipeline.addPass(PostOrderFunctionAttrsPass());
>>
>> Right - my point was that we could more easily explore doing so by having 
>> this new AlwaysInliner separate. In this patch I included those additional 
>> passes because I thought they may be necessary or beneficial, but I can 
>> remove them as a first step if they are not necessary. @jdoerfert - should 
>> the Attributor be run post-always inlining, or is it fine to not be run?
>
> I'd say start off without running any passes and keeping the status quo (i.e. 
> an NFC patch), then explore adding passes between the inliners in a future 
> patch.

Done

>>> And is there any evidence that running the function simplification pipeline 
>>> between the mandatory and full inliner is helpful? It could affect compile 
>>> times.
>>
>> In the ML-driven -Oz case, we saw some marginal improvement. I haven't in 
>> -O3 cases using the "non-ml" inliner. I suspect that it helps in the ML 
>> policy case (both -Oz and -O3, on which we're currently working), because: 
>> 1) the current -Oz takes some global (module-wide) features, so probably 
>> simplifying out the trivial cases helps; and 2) we plan on taking into 
>> consideration regions of the call graph, and the intuition is that 
>> eliminating the trivial cases (mandatory cases) would rise the visibility 
>> (for a training algorithm) of the non-trivial cases.
>>
>>> I'd think that adding the mandatory inliner right before the full inliner 
>>> in the same CGSCC pass manager would do the job. e.g. add it in 
>>> `ModuleInlinerWrapperPass::ModuleInlinerWrapperPass()` right before 
>>> `PM.addPass(InlinerPass());`
>>
>> It would, and what I'm proposing here is equivalent to that, but the 
>> proposal here helps with these other explorations, with (arguably) not much 
>> of a  difference cost-wise in itself (meaning, of course, if we discover 
>> there's benefit in running those additional passes, we pay with compile 
>> time, but in of itself, factoring the always inliner in its own wrapper, or 
>> in the same wrapper as the inliner, doesn't really come at much of a cost).
>>
>> Now, if we determine there is no value, we can bring it back easily - wdyt?
>
> I'll run this through llvm-compile-time-tracker to see what the compile time 
> implications are.

You mean for the variant where we ran some of the function passes, or you'd try 
running all of them? Probably the latter would be quite interesting as a 'worst 
case'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

>> I'll run this through llvm-compile-time-tracker to see what the compile time 
>> implications are.
>
> You mean for the variant where we ran some of the function passes, or you'd 
> try running all of them? Probably the latter would be quite interesting as a 
> 'worst case'.

I was trying the previous patch, but will also try running all function passes, 
definitely would be interesting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Google Contributors to LLVM via Phabricator via cfe-commits]

google-llvm-upstream-contributions added a comment.

In D91567#2403173 , @aeubanks wrote:

> In D91567#2400699 , @mtrofin wrote:
>
>> In D91567#2400207 , @aeubanks wrote:
>>
>>> What about removing the existing AlwaysInlinerPass and replacing it with 
>>> this one? Or is that something you were planning to do in a follow-up 
>>> change?
>>
>> That's the plan, yes.
>>
 open the opportunity to help the full inliner by performing additional 
 function passes after the mandatory inlinings, but before the full inliner
>>>
>>> This change doesn't run the function simplification pipeline between the 
>>> mandatory and full inliner though, only
>>>
>>>   if (AttributorRun & AttributorRunOption::CGSCC)
>>> MainCGPipeline.addPass(AttributorCGSCCPass());
>>>   
>>>   if (PTO.Coroutines)
>>> MainCGPipeline.addPass(CoroSplitPass(Level != OptimizationLevel::O0));
>>>   
>>>   // Now deduce any function attributes based in the current code.
>>>   MainCGPipeline.addPass(PostOrderFunctionAttrsPass());
>>
>> Right - my point was that we could more easily explore doing so by having 
>> this new AlwaysInliner separate. In this patch I included those additional 
>> passes because I thought they may be necessary or beneficial, but I can 
>> remove them as a first step if they are not necessary. @jdoerfert - should 
>> the Attributor be run post-always inlining, or is it fine to not be run?
>
> I'd say start off without running any passes and keeping the status quo (i.e. 
> an NFC patch), then explore adding passes between the inliners in a future 
> patch.

Done

>>> And is there any evidence that running the function simplification pipeline 
>>> between the mandatory and full inliner is helpful? It could affect compile 
>>> times.
>>
>> In the ML-driven -Oz case, we saw some marginal improvement. I haven't in 
>> -O3 cases using the "non-ml" inliner. I suspect that it helps in the ML 
>> policy case (both -Oz and -O3, on which we're currently working), because: 
>> 1) the current -Oz takes some global (module-wide) features, so probably 
>> simplifying out the trivial cases helps; and 2) we plan on taking into 
>> consideration regions of the call graph, and the intuition is that 
>> eliminating the trivial cases (mandatory cases) would rise the visibility 
>> (for a training algorithm) of the non-trivial cases.
>>
>>> I'd think that adding the mandatory inliner right before the full inliner 
>>> in the same CGSCC pass manager would do the job. e.g. add it in 
>>> `ModuleInlinerWrapperPass::ModuleInlinerWrapperPass()` right before 
>>> `PM.addPass(InlinerPass());`
>>
>> It would, and what I'm proposing here is equivalent to that, but the 
>> proposal here helps with these other explorations, with (arguably) not much 
>> of a  difference cost-wise in itself (meaning, of course, if we discover 
>> there's benefit in running those additional passes, we pay with compile 
>> time, but in of itself, factoring the always inliner in its own wrapper, or 
>> in the same wrapper as the inliner, doesn't really come at much of a cost).
>>
>> Now, if we determine there is no value, we can bring it back easily - wdyt?
>
> I'll run this through llvm-compile-time-tracker to see what the compile time 
> implications are.

You mean for the variant where we ran some of the function passes, or you'd try 
running all of them? Probably the latter would be quite interesting as a 'worst 
case'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin updated this revision to Diff 306141.
mtrofin added a comment.

Running just the always inliner variant, without other passes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

Files:
  clang/test/Frontend/optimization-remark-line-directive.c
  llvm/include/llvm/Analysis/InlineAdvisor.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Analysis/InlineAdvisor.cpp
  llvm/lib/Analysis/MLInlineAdvisor.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/Inliner.cpp
  llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
  llvm/test/Transforms/Inline/ML/bounds-checks.ll
  llvm/test/Transforms/Inline/inline_stats.ll

Index: llvm/test/Transforms/Inline/inline_stats.ll
===
--- llvm/test/Transforms/Inline/inline_stats.ll
+++ llvm/test/Transforms/Inline/inline_stats.ll
@@ -6,8 +6,11 @@
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
 
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
+
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
 
 ; CHECK: --- Dumping inliner stats for [] ---
 ; CHECK-BASIC-NOT: -- List of inlined functions:
Index: llvm/test/Transforms/Inline/ML/bounds-checks.ll
===
--- llvm/test/Transforms/Inline/ML/bounds-checks.ll
+++ llvm/test/Transforms/Inline/ML/bounds-checks.ll
@@ -4,7 +4,7 @@
 ; factor, we don't inline anymore.
 ; REQUIRES: have_tf_aot
 ; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=10.0 -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=NOBOUNDS
-; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=1.0 -disable-always-inliner-in-module-wrapper -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=BOUNDS
+; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=1.0 -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=BOUNDS
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-grtev4-linux-gnu"
Index: llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
===
--- llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
+++ llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
@@ -7,18 +7,18 @@
 ; REQUIRES: have_tf_api
 ;
 ; When the bounds are very wide ("no bounds"), all inlinings happen.
-; RUN: opt -passes=scc-oz-module-inliner -ml-inliner-ir2native-model=%S/../../../../unittests/Analysis/Inputs/ir2native_x86_64_model -ml-inliner-model-under-training=%S/../../../../lib/Analysis/models/inliner -training-log=- -enable-ml-inliner=development -ml-advisor-size-increase-threshold=10.0 -disable-always-inliner-in-module-wrapper -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=NOBOUNDS
+; RUN: opt -passes=scc-oz-module-inliner -ml-inliner-ir2native-model=%S/../../../../unittests/Analysis/Inputs/ir2native_x86_64_model -ml-inliner-model-under-training=%S/../../../../lib/Analysis/models/inliner -training-log=- -enable-ml-inliner=development -ml-advisor-size-increase-threshold=10.0 -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=NOBOUNDS
 ;
 ; When the bounds are very restrictive, the first inlining happens but it's
 ; considered as "bad" (since it trips over the bounds) and its reward is a
 ; penalty. However, the mandatory inlining, which is considered next, happens.
 ; No other inlinings happend.
-; RUN: opt -passes=scc-oz-module-inliner -ml-inliner-ir2native-model=%S/../../../../unittests/Analysis/Inputs/ir2native_x86_64_model -ml-inliner-model-under-training=%S/../../../../lib/Analysis/models/i

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D91567#2400699 , @mtrofin wrote:

> In D91567#2400207 , @aeubanks wrote:
>
>> What about removing the existing AlwaysInlinerPass and replacing it with 
>> this one? Or is that something you were planning to do in a follow-up change?
>
> That's the plan, yes.
>
>>> open the opportunity to help the full inliner by performing additional 
>>> function passes after the mandatory inlinings, but before the full inliner
>>
>> This change doesn't run the function simplification pipeline between the 
>> mandatory and full inliner though, only
>>
>>   if (AttributorRun & AttributorRunOption::CGSCC)
>> MainCGPipeline.addPass(AttributorCGSCCPass());
>>   
>>   if (PTO.Coroutines)
>> MainCGPipeline.addPass(CoroSplitPass(Level != OptimizationLevel::O0));
>>   
>>   // Now deduce any function attributes based in the current code.
>>   MainCGPipeline.addPass(PostOrderFunctionAttrsPass());
>
> Right - my point was that we could more easily explore doing so by having 
> this new AlwaysInliner separate. In this patch I included those additional 
> passes because I thought they may be necessary or beneficial, but I can 
> remove them as a first step if they are not necessary. @jdoerfert - should 
> the Attributor be run post-always inlining, or is it fine to not be run?

I'd say start off without running any passes and keeping the status quo (i.e. 
an NFC patch), then explore adding passes between the inliners in a future 
patch.

>> And is there any evidence that running the function simplification pipeline 
>> between the mandatory and full inliner is helpful? It could affect compile 
>> times.
>
> In the ML-driven -Oz case, we saw some marginal improvement. I haven't in -O3 
> cases using the "non-ml" inliner. I suspect that it helps in the ML policy 
> case (both -Oz and -O3, on which we're currently working), because: 1) the 
> current -Oz takes some global (module-wide) features, so probably simplifying 
> out the trivial cases helps; and 2) we plan on taking into consideration 
> regions of the call graph, and the intuition is that eliminating the trivial 
> cases (mandatory cases) would rise the visibility (for a training algorithm) 
> of the non-trivial cases.
>
>> I'd think that adding the mandatory inliner right before the full inliner in 
>> the same CGSCC pass manager would do the job. e.g. add it in 
>> `ModuleInlinerWrapperPass::ModuleInlinerWrapperPass()` right before 
>> `PM.addPass(InlinerPass());`
>
> It would, and what I'm proposing here is equivalent to that, but the proposal 
> here helps with these other explorations, with (arguably) not much of a  
> difference cost-wise in itself (meaning, of course, if we discover there's 
> benefit in running those additional passes, we pay with compile time, but in 
> of itself, factoring the always inliner in its own wrapper, or in the same 
> wrapper as the inliner, doesn't really come at much of a cost).
>
> Now, if we determine there is no value, we can bring it back easily - wdyt?

I'll run this through llvm-compile-time-tracker to see what the compile time 
implications are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Thanks for the walkthroughs/help. Also stared at the code a bit. I think I get 
it now. Some of the confusion also came from having both LPM and NPM versions 
of the always inliner in the same file, though they seem to share no code.

I'll leave the more nuanced review to folks more familiar with it - sorry for 
any noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin added a comment.

In D91567#2401021 , @dblaikie wrote:

> In D91567#2398637 , @mtrofin wrote:
>
>> In D91567#2398623 , @dblaikie wrote:
>>
>>> In D91567#2398461 , @mtrofin wrote:
>>>
 In D91567#2398440 , @dblaikie 
 wrote:

>> Performing the mandatory inlinings first simplifies the problem the full 
>> inliner needs to solve
>
> That confuses me a bit - is that suggesting that we don't run the 
> AlwaysInliner when we are running the Inliner (ie: we only run the 
> AlwaysInliner at -O0, and use the Inliner at higher optimization levels 
> and let the Inliner do always inlining too)?
> & sounds like this is suggesting that would change? That we would now 
> perform always inlining separately from inlining? Maybe that's an 
> orthogonal/separate change from one implementing the always inlining 
> using the Inliner being run in a separate mode?

 In the NPM, we didn't run the AlwaysInliner until D86988 
 . See also the discussion there. The 
 normal inliner pass was, and still is, taking care of the mandatory 
 inlinings if it finds them. Of course, if we completely upfronted those 
 (which this patch can do), then the normal inliner wouldn't need to. I'm 
 not suggesting changing that - meaning, it's straightforward for the 
 normal inliner to take care of mandatory and policy-driven inlinings. The 
 idea, though, is that if we upfront the mandatory inlinings, the shape of 
 the call graph the inliner operates over is simpler and the effects of 
 inlining probably more easy to glean by the decision making policy. There 
 are trade-offs, though - we can increase that "ease of gleaning" by 
 performing more function simplification passes between the mandatory 
 inlinings and the full inliner.
>>>
>>> OK, so if I understand correctly with the old Pass Manager there were two 
>>> separate passes (always inliner and inliner - they share some code though, 
>>> yeah?)
>>
>> AlwaysInlinerLegacyPass does, yes. The NPM variant doesn't.
>
> The NPM always inliner doesn't share any code with the NPM non-always 
> inliner? (though this ( https://reviews.llvm.org/D86988 ) is the patch that 
> added a separate always inliner to the NPM, right? And that patch doesn't 
> look like it adds a whole new pass implementation - so looks like it's 
> sharing some code with something?)

There was already an AlwaysInliner for the NPM, just wasn't used. So D86966 
 hooked that up in the NPM, basically. The 
implementation of that AlwaysInliner is separate from the Inliner pass. See 
Transforms/IPO/AlwaysInliner.cpp lines 36 - 114, vs Inliner.cpp, from 687 
onwards.

>>> and they were run in the pass pipeline but potentially (definitely?) not 
>>> adjacent?
>>
>> From what I can see, the legacy one was used only in the O0/O1 
>>  cases, see 
>> clang/lib/CodeGen/BackendUtil,cpp:643. The full inliner isn't.
>
> The full inliner isn't.. isn't run at -O0/-O1? So with the Legacy Pass 
> Manager one inliner (always or non-always) was used in a given compilation, 
> not both? (so I guess then the non-always inliner did the always-inlining in 
> -O2 and above in the old pass manager? But didn't have the same recursive 
> always inlining miss that the NPM non-always inliner had?)

Yup, see BackendUtil.cpp:634. Can't comment on the latter problem.

>> New pass manager survived for quite a while with only one inlining pass, 
>> that included a mandatorily strong preference for inlining always-inline 
>> functions? But still missed some recursive cases. So D86988 
>>  made the always inliner run right next 
>> to/before the inliner in the NPM.
>>
>>> Now there's tihs patch, to implement the AlwaysInliner using the inliner - 
>>> but is also changing the order of passes to improve optimization 
>>> opportunities by doing some cleanup after always inlining?
>>
>> It doesn't quite change the order D86988  
>> introduced. Specifically, D86988  ran 
>> AlwaysInliner (a module pass) first, then let the Inliner and function 
>> optimizations happen.
>> This patch keeps the order between doing mandatory inlinings and inlinings. 
>> But, in addition, if in the future we want to also perform some of the 
>> function passes that happen in the inliner case, to help the full inliner, 
>> we can more easily do so.
>
> I'm still a bit confused/trying to understand better - am I understanding 
> correctly when I say: D86988  added always 
> inlining (for the NPM) as a separate process within the non-always

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D91567#2398637 , @mtrofin wrote:

> In D91567#2398623 , @dblaikie wrote:
>
>> In D91567#2398461 , @mtrofin wrote:
>>
>>> In D91567#2398440 , @dblaikie 
>>> wrote:
>>>
> Performing the mandatory inlinings first simplifies the problem the full 
> inliner needs to solve

 That confuses me a bit - is that suggesting that we don't run the 
 AlwaysInliner when we are running the Inliner (ie: we only run the 
 AlwaysInliner at -O0, and use the Inliner at higher optimization levels 
 and let the Inliner do always inlining too)?
 & sounds like this is suggesting that would change? That we would now 
 perform always inlining separately from inlining? Maybe that's an 
 orthogonal/separate change from one implementing the always inlining using 
 the Inliner being run in a separate mode?
>>>
>>> In the NPM, we didn't run the AlwaysInliner until D86988 
>>> . See also the discussion there. The 
>>> normal inliner pass was, and still is, taking care of the mandatory 
>>> inlinings if it finds them. Of course, if we completely upfronted those 
>>> (which this patch can do), then the normal inliner wouldn't need to. I'm 
>>> not suggesting changing that - meaning, it's straightforward for the normal 
>>> inliner to take care of mandatory and policy-driven inlinings. The idea, 
>>> though, is that if we upfront the mandatory inlinings, the shape of the 
>>> call graph the inliner operates over is simpler and the effects of inlining 
>>> probably more easy to glean by the decision making policy. There are 
>>> trade-offs, though - we can increase that "ease of gleaning" by performing 
>>> more function simplification passes between the mandatory inlinings and the 
>>> full inliner.
>>
>> OK, so if I understand correctly with the old Pass Manager there were two 
>> separate passes (always inliner and inliner - they share some code though, 
>> yeah?)
>
> AlwaysInlinerLegacyPass does, yes. The NPM variant doesn't.

The NPM always inliner doesn't share any code with the NPM non-always inliner? 
(though this ( https://reviews.llvm.org/D86988 ) is the patch that added a 
separate always inliner to the NPM, right? And that patch doesn't look like it 
adds a whole new pass implementation - so looks like it's sharing some code 
with something?)

>> and they were run in the pass pipeline but potentially (definitely?) not 
>> adjacent?
>
> From what I can see, the legacy one was used only in the O0/O1 
>  cases, see 
> clang/lib/CodeGen/BackendUtil,cpp:643. The full inliner isn't.

The full inliner isn't.. isn't run at -O0/-O1? So with the Legacy Pass Manager 
one inliner (always or non-always) was used in a given compilation, not both? 
(so I guess then the non-always inliner did the always-inlining in -O2 and 
above in the old pass manager? But didn't have the same recursive always 
inlining miss that the NPM non-always inliner had?)

> New pass manager survived for quite a while with only one inlining pass, that 
> included a mandatorily strong preference for inlining always-inline 
> functions? But still missed some recursive cases. So D86988 
>  made the always inliner run right next 
> to/before the inliner in the NPM.
>
>> Now there's tihs patch, to implement the AlwaysInliner using the inliner - 
>> but is also changing the order of passes to improve optimization 
>> opportunities by doing some cleanup after always inlining?
>
> It doesn't quite change the order D86988  
> introduced. Specifically, D86988  ran 
> AlwaysInliner (a module pass) first, then let the Inliner and function 
> optimizations happen.
> This patch keeps the order between doing mandatory inlinings and inlinings. 
> But, in addition, if in the future we want to also perform some of the 
> function passes that happen in the inliner case, to help the full inliner, we 
> can more easily do so.

I'm still a bit confused/trying to understand better - am I understanding 
correctly when I say: D86988  added always 
inlining (for the NPM) as a separate process within the non-always inliner? And 
this patch you're proposing. breaks always inlining out into a separate pass 
proper, so that at some point, if someone wanted to (but not being done in this 
patch) they could put some passes in between the two runs of inlining (always 
and non-always)?

(I guess one thing I might be especially confused about is the "reuse X to do 
Y" would, to me, immediately lead me to think about "so I expect to see a bunch 
of deleted code because X presumably was doing a bunch of stuff itself that it 
now doesn't have to" - but I guess that's not

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin added a comment.

In D91567#2400207 , @aeubanks wrote:

> What about removing the existing AlwaysInlinerPass and replacing it with this 
> one? Or is that something you were planning to do in a follow-up change?

That's the plan, yes.

>> open the opportunity to help the full inliner by performing additional 
>> function passes after the mandatory inlinings, but before the full inliner
>
> This change doesn't run the function simplification pipeline between the 
> mandatory and full inliner though, only
>
>   if (AttributorRun & AttributorRunOption::CGSCC)
> MainCGPipeline.addPass(AttributorCGSCCPass());
>   
>   if (PTO.Coroutines)
> MainCGPipeline.addPass(CoroSplitPass(Level != OptimizationLevel::O0));
>   
>   // Now deduce any function attributes based in the current code.
>   MainCGPipeline.addPass(PostOrderFunctionAttrsPass());

Right - my point was that we could more easily explore doing so by having this 
new AlwaysInliner separate. In this patch I included those additional passes 
because I thought they may be necessary or beneficial, but I can remove them as 
a first step if they are not necessary. @jdoerfert - should the Attributor be 
run post-always inlining, or is it fine to not be run?

> And is there any evidence that running the function simplification pipeline 
> between the mandatory and full inliner is helpful? It could affect compile 
> times.

In the ML-driven -Oz case, we saw some marginal improvement. I haven't in -O3 
cases using the "non-ml" inliner. I suspect that it helps in the ML policy case 
(both -Oz and -O3, on which we're currently working), because: 1) the current 
-Oz takes some global (module-wide) features, so probably simplifying out the 
trivial cases helps; and 2) we plan on taking into consideration regions of the 
call graph, and the intuition is that eliminating the trivial cases (mandatory 
cases) would rise the visibility (for a training algorithm) of the non-trivial 
cases.

> I'd think that adding the mandatory inliner right before the full inliner in 
> the same CGSCC pass manager would do the job. e.g. add it in 
> `ModuleInlinerWrapperPass::ModuleInlinerWrapperPass()` right before 
> `PM.addPass(InlinerPass());`

It would, and what I'm proposing here is equivalent to that, but the proposal 
here helps with these other explorations, with (arguably) not much of a  
difference cost-wise in itself (meaning, of course, if we discover there's 
benefit in running those additional passes, we pay with compile time, but in of 
itself, factoring the always inliner in its own wrapper, or in the same wrapper 
as the inliner, doesn't really come at much of a cost).

Now, if we determine there is no value, we can bring it back easily - wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

What about removing the existing AlwaysInlinerPass and replacing it with this 
one? Or is that something you were planning to do in a follow-up change?

> open the opportunity to help the full inliner by performing additional 
> function passes after the mandatory inlinings, but before the full inliner

This change doesn't run the function simplification pipeline between the 
mandatory and full inliner though, only

  if (AttributorRun & AttributorRunOption::CGSCC)
MainCGPipeline.addPass(AttributorCGSCCPass());
  
  if (PTO.Coroutines)
MainCGPipeline.addPass(CoroSplitPass(Level != OptimizationLevel::O0));
  
  // Now deduce any function attributes based in the current code.
  MainCGPipeline.addPass(PostOrderFunctionAttrsPass());

And is there any evidence that running the function simplification pipeline 
between the mandatory and full inliner is helpful? It could affect compile 
times.

I'd think that adding the mandatory inliner right before the full inliner in 
the same CGSCC pass manager would do the job. e.g. add it in 
`ModuleInlinerWrapperPass::ModuleInlinerWrapperPass()` right before 
`PM.addPass(InlinerPass());`




Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:27
 
 /// There are 3 scenarios we can use the InlineAdvisor:
 /// - Default - use manual heuristics.

4


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin added a comment.

In D91567#2398623 , @dblaikie wrote:

> In D91567#2398461 , @mtrofin wrote:
>
>> In D91567#2398440 , @dblaikie wrote:
>>
 Performing the mandatory inlinings first simplifies the problem the full 
 inliner needs to solve
>>>
>>> That confuses me a bit - is that suggesting that we don't run the 
>>> AlwaysInliner when we are running the Inliner (ie: we only run the 
>>> AlwaysInliner at -O0, and use the Inliner at higher optimization levels and 
>>> let the Inliner do always inlining too)?
>>> & sounds like this is suggesting that would change? That we would now 
>>> perform always inlining separately from inlining? Maybe that's an 
>>> orthogonal/separate change from one implementing the always inlining using 
>>> the Inliner being run in a separate mode?
>>
>> In the NPM, we didn't run the AlwaysInliner until D86988 
>> . See also the discussion there. The normal 
>> inliner pass was, and still is, taking care of the mandatory inlinings if it 
>> finds them. Of course, if we completely upfronted those (which this patch 
>> can do), then the normal inliner wouldn't need to. I'm not suggesting 
>> changing that - meaning, it's straightforward for the normal inliner to take 
>> care of mandatory and policy-driven inlinings. The idea, though, is that if 
>> we upfront the mandatory inlinings, the shape of the call graph the inliner 
>> operates over is simpler and the effects of inlining probably more easy to 
>> glean by the decision making policy. There are trade-offs, though - we can 
>> increase that "ease of gleaning" by performing more function simplification 
>> passes between the mandatory inlinings and the full inliner.
>
> OK, so if I understand correctly with the old Pass Manager there were two 
> separate passes (always inliner and inliner - they share some code though, 
> yeah?)

AlwaysInlinerLegacyPass does, yes. The NPM variant doesn't.

> and they were run in the pass pipeline but potentially (definitely?) not 
> adjacent?

From what I can see, the legacy one was used only in the O0/O1 
 cases, see 
clang/lib/CodeGen/BackendUtil,cpp:643. The full inliner isn't.

New pass manager survived for quite a while with only one inlining pass, that 
included a mandatorily strong preference for inlining always-inline functions? 
But still missed some recursive cases. So D86988 
 made the always inliner run right next 
to/before the inliner in the NPM.

> Now there's tihs patch, to implement the AlwaysInliner using the inliner - 
> but is also changing the order of passes to improve optimization 
> opportunities by doing some cleanup after always inlining?

It doesn't quite change the order D86988  
introduced. Specifically, D86988  ran 
AlwaysInliner (a module pass) first, then let the Inliner and function 
optimizations happen.
This patch keeps the order between doing mandatory inlinings and inlinings. 
But, in addition, if in the future we want to also perform some of the function 
passes that happen in the inliner case, to help the full inliner, we can more 
easily do so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D91567#2398461 , @mtrofin wrote:

> In D91567#2398440 , @dblaikie wrote:
>
>>> Performing the mandatory inlinings first simplifies the problem the full 
>>> inliner needs to solve
>>
>> That confuses me a bit - is that suggesting that we don't run the 
>> AlwaysInliner when we are running the Inliner (ie: we only run the 
>> AlwaysInliner at -O0, and use the Inliner at higher optimization levels and 
>> let the Inliner do always inlining too)?
>> & sounds like this is suggesting that would change? That we would now 
>> perform always inlining separately from inlining? Maybe that's an 
>> orthogonal/separate change from one implementing the always inlining using 
>> the Inliner being run in a separate mode?
>
> In the NPM, we didn't run the AlwaysInliner until D86988 
> . See also the discussion there. The normal 
> inliner pass was, and still is, taking care of the mandatory inlinings if it 
> finds them. Of course, if we completely upfronted those (which this patch can 
> do), then the normal inliner wouldn't need to. I'm not suggesting changing 
> that - meaning, it's straightforward for the normal inliner to take care of 
> mandatory and policy-driven inlinings. The idea, though, is that if we 
> upfront the mandatory inlinings, the shape of the call graph the inliner 
> operates over is simpler and the effects of inlining probably more easy to 
> glean by the decision making policy. There are trade-offs, though - we can 
> increase that "ease of gleaning" by performing more function simplification 
> passes between the mandatory inlinings and the full inliner.

OK, so if I understand correctly with the old Pass Manager there were two 
separate passes (always inliner and inliner - they share some code though, 
yeah?) and they were run in the pass pipeline but potentially (definitely?) not 
adjacent? New pass manager survived for quite a while with only one inlining 
pass, that included a mandatorily strong preference for inlining always-inline 
functions? But still missed some recursive cases. So D86988 
 made the always inliner run right next 
to/before the inliner in the NPM.

Now there's tihs patch, to implement the AlwaysInliner using the inliner - but 
is also changing the order of passes to improve optimization opportunities by 
doing some cleanup after always inlining?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin added a comment.

In D91567#2398440 , @dblaikie wrote:

>> Performing the mandatory inlinings first simplifies the problem the full 
>> inliner needs to solve
>
> That confuses me a bit - is that suggesting that we don't run the 
> AlwaysInliner when we are running the Inliner (ie: we only run the 
> AlwaysInliner at -O0, and use the Inliner at higher optimization levels and 
> let the Inliner do always inlining too)?
> & sounds like this is suggesting that would change? That we would now perform 
> always inlining separately from inlining? Maybe that's an orthogonal/separate 
> change from one implementing the always inlining using the Inliner being run 
> in a separate mode?

In the NPM, we didn't run the AlwaysInliner until D86988 
. See also the discussion there. The normal 
inliner pass was, and still is, taking care of the mandatory inlinings if it 
finds them. Of course, if we completely upfronted those (which this patch can 
do), then the normal inliner wouldn't need to. I'm not suggesting changing that 
- meaning, it's straightforward for the normal inliner to take care of 
mandatory and policy-driven inlinings. The idea, though, is that if we upfront 
the mandatory inlinings, the shape of the call graph the inliner operates over 
is simpler and the effects of inlining probably more easy to glean by the 
decision making policy. There are trade-offs, though - we can increase that 
"ease of gleaning" by performing more function simplification passes between 
the mandatory inlinings and the full inliner.

In D91567#2398440 , @dblaikie wrote:

>> Performing the mandatory inlinings first simplifies the problem the full 
>> inliner needs to solve
>
> That confuses me a bit - is that suggesting that we don't run the 
> AlwaysInliner when we are running the Inliner (ie: we only run the 
> AlwaysInliner at -O0, and use the Inliner at higher optimization levels and 
> let the Inliner do always inlining too)?
> & sounds like this is suggesting that would change? That we would now perform 
> always inlining separately from inlining? Maybe that's an orthogonal/separate 
> change from one implementing the always inlining using the Inliner being run 
> in a separate mode?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

> Performing the mandatory inlinings first simplifies the problem the full 
> inliner needs to solve

That confuses me a bit - is that suggesting that we don't run the AlwaysInliner 
when we are running the Inliner (ie: we only run the AlwaysInliner at -O0, and 
use the Inliner at higher optimization levels and let the Inliner do always 
inlining too)?
& sounds like this is suggesting that would change? That we would now perform 
always inlining separately from inlining? Maybe that's an orthogonal/separate 
change from one implementing the always inlining using the Inliner being run in 
a separate mode?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin added a comment.

Please note: the patch isn't 100% ready, there are those tests that check how 
the pipeline is composed, which are unpleasant to fix, so I want to defer them 
to after we get agreement over the larger points this patch brings (i.e. 
pre-performing always inlinings, value in further exploring cleanups before 
full inlining, etc)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin created this revision.
mtrofin added reviewers: aeubanks, jdoerfert, davidxl, eraman.
Herald added subscribers: llvm-commits, cfe-commits, hiraditya.
Herald added projects: clang, LLVM.
mtrofin requested review of this revision.

Enable performing mandatory inlinings upfront, by reusing the same logic
as the full inliner, instead of the AlwaysInliner. This has the
following benefits:

- reduce code duplication - one inliner codebase
- open the opportunity to help the full inliner by performing additional

function passes after the mandatory inlinings, but before th full
inliner. Performing the mandatory inlinings first simplifies the problem
the full inliner needs to solve: less call sites, more contextualization, and,
depending on the additional function optimization passes run between the
2 inliners, higher accuracy of cost models / decision policies.

Note that this patch does not yet enable much in terms of post-always
inline function optimization.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91567

Files:
  clang/test/Frontend/optimization-remark-line-directive.c
  llvm/include/llvm/Analysis/InlineAdvisor.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Analysis/InlineAdvisor.cpp
  llvm/lib/Analysis/MLInlineAdvisor.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/Inliner.cpp
  llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
  llvm/test/Transforms/Inline/ML/bounds-checks.ll
  llvm/test/Transforms/Inline/inline_stats.ll

Index: llvm/test/Transforms/Inline/inline_stats.ll
===
--- llvm/test/Transforms/Inline/inline_stats.ll
+++ llvm/test/Transforms/Inline/inline_stats.ll
@@ -6,8 +6,11 @@
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
 ; RUN: opt -S -passes=inline -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
 
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
-; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=CHECK-BASIC -check-prefix=CHECK
+; RUN: opt -S -passes=inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix="CHECK-VERBOSE" -check-prefix=CHECK
+
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=basic < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-BASIC -check-prefix=WRAPPER
+; RUN: opt -S -passes=always-inliner-wrapper,inliner-wrapper -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s -check-prefix=WRAPPER-VERBOSE -check-prefix=WRAPPER
 
 ; CHECK: --- Dumping inliner stats for [] ---
 ; CHECK-BASIC-NOT: -- List of inlined functions:
Index: llvm/test/Transforms/Inline/ML/bounds-checks.ll
===
--- llvm/test/Transforms/Inline/ML/bounds-checks.ll
+++ llvm/test/Transforms/Inline/ML/bounds-checks.ll
@@ -4,7 +4,7 @@
 ; factor, we don't inline anymore.
 ; REQUIRES: have_tf_aot
 ; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=10.0 -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=NOBOUNDS
-; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=1.0 -disable-always-inliner-in-module-wrapper -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=BOUNDS
+; RUN: opt -passes=scc-oz-module-inliner -enable-ml-inliner=release -ml-advisor-size-increase-threshold=1.0 -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=BOUNDS
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-grtev4-linux-gnu"
Index: llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
===
--- llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
+++ llvm/test/Transforms/Inline/ML/bounds-checks-rewards.ll
@@ -7,18 +7,18 @@
 ; REQUIRES: have_tf_api
 ;
 ; When the bounds are very wide ("no bounds"), all inlinings happen.
-; RUN: opt -passes=scc-oz-module-inliner -ml-inliner-ir2native-model=%S/../../../../unittests/Analysis/Inputs/ir2native_x86_64_model -ml-inliner-model-under-training=%S/../../../../lib/Analysis/models/inliner -training-log=- -enable-ml-inliner=development -ml-advisor-size-increase-threshold=10.0 -disable-always-inliner-in-module-wrapper -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=NOBOUNDS
+; RUN: opt -passes=scc-oz-module-inliner -ml-inliner-ir2native-mod