[PATCH] D18013: Using MPI for Profiling Data Reduction

[Hal Finkel via llvm-commits]

----- 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 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? 

-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 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160309/64bbe2a1/attachment.html>