[PATCH] D18013: Using MPI for Profiling Data Reduction

[Hal Finkel via llvm-commits]

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

> 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). 

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