[PATCH] D18013: Using MPI for Profiling Data Reduction

Wed Mar 9 17:36:00 PST 2016

----- Original Message -----

> 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 

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/c3aa5855/attachment.html>