[llvm] r308078 - [ThinLTO] Ensure we always select the same function copy to import

Sat Jul 15 06:39:34 PDT 2017

On Sat, Jul 15, 2017 at 12:53 AM Teresa Johnson via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: tejohnson
> Date: Fri Jul 14 21:53:05 2017
> New Revision: 308078
>
> URL: http://llvm.org/viewvc/llvm-project?rev=308078&view=rev
> Log:
> [ThinLTO] Ensure we always select the same function copy to import
>
> Summary:
> Check if the first eligible callee is under the instruction threshold.
> Checking this on the first eligible callee ensures that we don't end
> up selecting different callees to import when we invoke this routine
> with different thresholds due to reaching the callee via paths that
> are shallower or hotter (when there are multiple copies, i.e. with
> weak or linkonce linkage). We don't want to leave the decision of which
> copy to import up to the backend.
>
> Reviewers: mehdi_amini
>
> Subscribers: inglorion, fhahn, llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D35436
>
> Added:
>
> llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_resolved1.ll
>
> llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_resolved2.ll
>     llvm/trunk/test/Transforms/FunctionImport/funcimport_resolved.ll
>

This newly added test appears to be flaky, showing some nondeterminism in
the system. For example, this command will run the tests 100 times, and
doing this I see roughly 5 failures:

% repeat 100 ./dev/bin/llvm-lit -s ../test/Transforms/FunctionImport

The build bots have been failing and then recovering because of this for
some time, so I'm going to revert until we can get this fixed to lessen the
build bot spam.

Modified:
>     llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
>
> Modified: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp?rev=308078&r1=308077&r2=308078&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp (original)
> +++ llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp Fri Jul 14 21:53:05
> 2017
> @@ -125,6 +125,7 @@ static const GlobalValueSummary *
>  selectCallee(const ModuleSummaryIndex &Index,
>               ArrayRef<std::unique_ptr<GlobalValueSummary>>
> CalleeSummaryList,
>               unsigned Threshold, StringRef CallerModulePath) {
> +  // Find the first eligible callee (e.g. legality checks).
>    auto It = llvm::find_if(
>        CalleeSummaryList,
>        [&](const std::unique_ptr<GlobalValueSummary> &SummaryPtr) {
> @@ -160,9 +161,6 @@ selectCallee(const ModuleSummaryIndex &I
>              Summary->modulePath() != CallerModulePath)
>            return false;
>
> -        if (Summary->instCount() > Threshold)
> -          return false;
> -
>          if (Summary->notEligibleToImport())
>            return false;
>
> @@ -171,7 +169,19 @@ selectCallee(const ModuleSummaryIndex &I
>    if (It == CalleeSummaryList.end())
>      return nullptr;
>
> -  return cast<GlobalValueSummary>(It->get());
> +  // Now check if the first eligible callee is under the instruction
> +  // threshold. Checking this on the first eligible callee ensures that
> +  // we don't end up selecting different callees to import when we invoke
> +  // this routine with different thresholds (when there are multiple
> copies,
> +  // i.e. with weak or linkonce linkage).
> +  auto *Summary = dyn_cast<FunctionSummary>(It->get());
> +  if (auto *AS = dyn_cast<AliasSummary>(It->get()))
> +    Summary = cast<FunctionSummary>(&AS->getAliasee());
> +  assert(Summary && "Expected FunctionSummary, or alias to one");
> +  if (Summary->instCount() > Threshold)
> +    return nullptr;
> +
> +  return Summary;
>  }
>
>  using EdgeInfo = std::tuple<const FunctionSummary *, unsigned /*
> Threshold */,
>
> Added:
> llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_resolved1.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_resolved1.ll?rev=308078&view=auto
>
> ==============================================================================
> ---
> llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_resolved1.ll
> (added)
> +++
> llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_resolved1.ll
> Fri Jul 14 21:53:05 2017
> @@ -0,0 +1,34 @@
> +target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
> +target triple = "x86_64-apple-macosx10.11.0"
> +
> +define void @foo() {
> +  call void @linkonceodrfunc()
> +  call void @linkonceodrfunc2()
> +  ret void
> +}
> +
> +define linkonce_odr void @linkonceodrfunc() {
> +  call void @f()
> +  call void @f()
> +  call void @f()
> +  call void @f()
> +  call void @f()
> +  call void @f()
> +  call void @f()
> +  ret void
> +}
> +
> +define linkonce_odr void @linkonceodrfunc2() {
> +  call void @f()
> +  call void @f()
> +  call void @f()
> +  call void @f()
> +  call void @f()
> +  call void @f()
> +  call void @f()
> +  ret void
> +}
> +
> +define internal void @f() {
> +  ret void
> +}
>
> Added:
> llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_resolved2.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_resolved2.ll?rev=308078&view=auto
>
> ==============================================================================
> ---
> llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_resolved2.ll
> (added)
> +++
> llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_resolved2.ll
> Fri Jul 14 21:53:05 2017
> @@ -0,0 +1,6 @@
> +target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
> +target triple = "x86_64-apple-macosx10.11.0"
> +
> +define linkonce_odr void @linkonceodrfunc() {
> +  ret void
> +}
>
> Added: llvm/trunk/test/Transforms/FunctionImport/funcimport_resolved.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionImport/funcimport_resolved.ll?rev=308078&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/FunctionImport/funcimport_resolved.ll
> (added)
> +++ llvm/trunk/test/Transforms/FunctionImport/funcimport_resolved.ll Fri
> Jul 14 21:53:05 2017
> @@ -0,0 +1,43 @@
> +; Test to ensure that we always select the same copy of a linkonce
> function
> +; when it is encountered with different thresholds. Initially, we will
> encounter
> +; the copy in funcimport_resolved1.ll with a lower threshold by reaching
> it
> +; from the deeper call chain via foo(), and it won't be selected for
> importing.
> +; Later we encounter it with a higher threshold via the direct call from
> main()
> +; and it will be selected. We don't want to select both the copy from
> +; funcimport_resolved1.ll and the smaller one from
> funcimport_resolved2.ll,
> +; leaving it up to the backend to figure out which one to actually import.
> +; The linkonce_odr may have different instruction counts in practice due
> to
> +; different inlines in the compile step.
> +
> +; RUN: opt -module-summary %s -o %t.bc
> +; RUN: opt -module-summary %p/Inputs/funcimport_resolved1.ll -o %t2.bc
> +; RUN: opt -module-summary %p/Inputs/funcimport_resolved2.ll -o %t3.bc
> +
> +; First do a sanity check that all callees are imported with the default
> +; instruction limit
> +; RUN: llvm-lto2 run %t.bc %t2.bc %t3.bc -o %t4 -r=%t.bc,_main,pl
> -r=%t.bc,_linkonceodrfunc,l -r=%t.bc,_foo,l -r=%t2.bc,_foo,pl
> -r=%t2.bc,_linkonceodrfunc,pl -r=%t2.bc,_linkonceodrfunc2,pl
> -r=%t3.bc,_linkonceodrfunc,l -debug-only=function-import 2>&1 | FileCheck
> %s --check-prefix=INSTLIMDEFAULT
> +; INSTLIMDEFAULT-DAG: Is importing function {{.*}} foo from
> {{.*}}funcimport_resolved1.ll
> +; INSTLIMDEFAULT-DAG: Is importing function {{.*}} linkonceodrfunc from
> {{.*}}funcimport_resolved1.ll
> +; INSTLIMDEFAULT-DAG: Is importing function {{.*}} linkonceodrfunc2 from
> {{.*}}funcimport_resolved1.ll
> +; INSTLIMDEFAULT-DAG: Is importing function {{.*}} f from
> {{.*}}funcimport_resolved1.ll
> +
> +; Now run with the lower threshold that will only allow linkonceodrfunc
> to be
> +; imported from funcimport_resolved1.ll the second time it is encountered.
> +; RUN: llvm-lto2 run %t.bc %t2.bc %t3.bc -o %t4 -r=%t.bc,_main,pl
> -r=%t.bc,_linkonceodrfunc,l -r=%t.bc,_foo,l -r=%t2.bc,_foo,pl
> -r=%t2.bc,_linkonceodrfunc,pl -r=%t2.bc,_linkonceodrfunc2,pl
> -r=%t3.bc,_linkonceodrfunc,l -debug-only=function-import
> -import-instr-limit=8 2>&1 | FileCheck %s --check-prefix=INSTLIM8
> +; INSTLIM8-DAG: Is importing function {{.*}} foo from
> {{.*}}funcimport_resolved1.ll
> +; INSTLIM8-DAG: Is importing function {{.*}} linkonceodrfunc from
> {{.*}}funcimport_resolved1.ll
> +; INSTLIM8-DAG: Not importing function {{.*}} linkonceodrfunc2 from
> {{.*}}funcimport_resolved1.ll
> +; INSTLIM8-DAG: Is importing function {{.*}} f from
> {{.*}}funcimport_resolved1.ll
> +
> +target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
> +target triple = "x86_64-apple-macosx10.11.0"
> +
> +define i32 @main() #0 {
> +entry:
> +  call void (...) @foo()
> +  call void (...) @linkonceodrfunc()
> +  ret i32 0
> +}
> +
> +declare void @foo(...) #1
> +declare void @linkonceodrfunc(...) #1
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170715/a87199d2/attachment.html>