[PATCH] D18013: Using MPI for Profiling Data Reduction

[Xinliang David Li via llvm-commits]

On Wed, Mar 9, 2016 at 4:47 PM, Hal Finkel <hfinkel at anl.gov> wrote:

>
>
> ------------------------------
>
> *From: *"Xinliang David Li" <davidxl at google.com>
> *To: *reviews+D18013+public+36299b239a77f675 at reviews.llvm.org
> *Cc: *"Daniel Waters" <dwatersg at gmail.com>, "Hal Finkel" <hfinkel at anl.gov>,
> "Vedant Kumar" <vsk at apple.com>, "Eric Christopher" <echristo at gmail.com>,
> "Duncan P. N. Exon Smith" <dexonsmith at apple.com>, "Sean Silva" <
> chisophugis at gmail.com>, "llvm-commits" <llvm-commits at lists.llvm.org>
> *Sent: *Wednesday, March 9, 2016 6:41:45 PM
> *Subject: *Re: [PATCH] D18013: Using MPI for Profiling Data Reduction
>
>
>
>
> On Wed, Mar 9, 2016 at 4:35 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>> silvas added a subscriber: silvas.
>>
>> ================
>> Comment at: lib/profile/InstrProfilingFile.c:86
>> @@ -74,2 +85,3 @@
>>    const char *Filename;
>> +#ifdef _MSC_VER
>>    FILE *File;
>> ----------------
>> Let's split this change into a separate patch for a focused discussion on
>> the requirements and possible solutions.
>>
>> ================
>> Comment at: lib/profile/InstrProfilingReduce.c:1
>> @@ +1,2 @@
>> +/*===- InstrProfilingReduce.c - Support library for applications using
>> MPI ===*\
>> +|*
>> ----------------
>> Let's call this InstrProfilingMPI.c consistent with the comment.
>>
>> ================
>> Comment at: lib/profile/InstrProfilingStub.c:14
>> @@ +13,2 @@
>> +void MPI_Reduce() { abort(); }
>> +void PMPI_Finalize() { abort(); }
>> ----------------
>> davidxl wrote:
>> > Is the MPI library defining thse functions  static or a DSO? If the
>> former, then perhaps declare those stubs weak so that we don't rely on the
>> link order of the runtime library
>> Why do we need these stubs?
>>
>>
> I thought it is needed when the binary is not linked with mpi library, but
> InstrProfilingReduce.o is linked in. Now that you asked, it seems that for
> that case, InstrProfilingReduce.o should not be linked in at all.
>
> I think the culprit is that __llvm_write_profile_data flag is defined in
> that file. With my suggestion to move it to InstrProfiling.c,  I think
> there is no need for the stub file. Good catch.
>
> It is __llvm_write_profile_data, but that's on purpose. The problem is
> that we need a hard dependency on InstrProfilingReduce.o in order to force
> the linker to bring it in. Otherwise, because there is already a weak
> definition of MPI_Finalize in the MPI library, the linker would not bring
> in InstrProfilingReduce.o at all and our MPI_Finalize implementation would
> never be called.
>
> You're correct that we then need the stubs to make linking succeed when
> we're not actually linking in an MPI library (e.g. for every regression
> test).
>
>
ok. Can those stubs be declared weak and merged into InstrProfilingReduce.c
?

David


> Thanks again,
> Hal
>
>
> David
>
>
>
>> http://reviews.llvm.org/D18013
>>
>>
>>
>>
>
>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160309/97c16c03/attachment.html>