[llvm] r308078 - [ThinLTO] Ensure we always select the same function copy to import
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Sat Jul 15 07:06:56 PDT 2017
Sorry about that, thanks for reverting. I just logged back on and saw this
myself. I know what is likely happening, but will make sure to run it a
bunch of times after fixing to make sure the problem is indeed fixed.
Teresa
On Sat, Jul 15, 2017 at 6:39 AM, Chandler Carruth <chandlerc at gmail.com>
wrote:
> 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
>>
>
--
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170715/e6aae4d4/attachment.html>
More information about the llvm-commits
mailing list