[PATCH] D18013: Using MPI for Profiling Data Reduction

Wed Mar 9 20:00:16 PST 2016

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

>
>
> ------------------------------
>
> *From: *"Xinliang David Li" <davidxl at google.com>
> *To: *"Hal Finkel" <hfinkel at anl.gov>
> *Cc: *"Daniel Waters" <dwatersg at gmail.com>, "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>,
> reviews+D18013+public+36299b239a77f675 at reviews.llvm.org
> *Sent: *Wednesday, March 9, 2016 7:22:18 PM
>
> *Subject: *Re: [PATCH] D18013: Using MPI for Profiling Data Reduction
>
>
>
> On Wed, Mar 9, 2016 at 5:14 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>
>>
>>
>> ------------------------------
>>
>> *From: *"Xinliang David Li" <davidxl at google.com>
>> *To: *"Hal Finkel" <hfinkel at anl.gov>
>> *Cc: *"Daniel Waters" <dwatersg at gmail.com>, "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>,
>> reviews+D18013+public+36299b239a77f675 at reviews.llvm.org
>> *Sent: *Wednesday, March 9, 2016 6:59:39 PM
>>
>> *Subject: *Re: [PATCH] D18013: Using MPI for Profiling Data Reduction
>>
>>
>>
>> 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 ?
>>
>> I don't think so because the MPI functions in the actual MPI library
>> themselves are weak and the weak vs. weak resolving is likely ill-defined.
>>
>> One option might be that, in our MPI_Finalize implementation, we *only*
>> use the "strong" PMPI_* variants. Then having weak stubs might work. What
>> do you think?
>>
>
> That will be ideal.  Are the 'weak'/'strong' binding for those functions
> well specified in the standard (can be relied upon)?
>
>
> The specification does not mandate the use of weak symbols (specifically
> because MPI can otherwise work on systems that have no such concept),
> however, weak symbols are specifically called out at the preferred
> implementation technique for systems that support them (and they're used in
> every implementation I've used). The discussion in the MPI specification is
> here:
>
>   http://www.mpi-forum.org/docs/mpi-3.1/mpi31-report/node364.htm#Node365
>
>
Consider it de facto standard then.

thanks,

David

> Thanks again,
> Hal
>
> David
>
>
>>
>>
>>  -Hal
>>
>>
>> David
>>
>>
>>> Thanks again,
>>> Hal
>>>
>>>
>>> David
>>>
>>>
>>>
>>>> http://reviews.llvm.org/D18013
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Hal Finkel
>>> Assistant Computational Scientist
>>> Leadership Computing Facility
>>> Argonne National Laboratory
>>>
>>
>>
>>
>>
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>>
>
>
>
>
> --
> 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/38cfb18f/attachment.html>