[PATCH] D18013: Using MPI for Profiling Data Reduction

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/profile/InstrProfilingFile.c:20
@@ +19,3 @@
+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/types.h>
----------------
is fcntl.h needed here?

================
Comment at: lib/profile/InstrProfilingFile.c:27
@@ -19,1 +26,3 @@
 
+#ifdef HAVE_MPI_H
+COMPILER_RT_VISIBILITY extern int __llvm_write_prof_data;
----------------
This seems like a good variable to have unconditionally. Put the declaration in InstrProfiling.h file.

================
Comment at: lib/profile/InstrProfilingFile.c:105
@@ -89,2 +104,3 @@
+#ifdef _MSC_VER
   /* Truncate the file.  Later we'll reopen and append. */
   File = fopen(Filename, "w");
----------------
The behavior divergence between windows and other platforms is not desirable. How about using 

_open(Filename, _O_WRONLY | _O_TRUNC)
...

================
Comment at: lib/profile/InstrProfilingFile.c:113
@@ +112,3 @@
+     In MPI applications, a file will only be created by one process */
+  File = open(Filename, O_WRONLY | O_TRUNC);
+  if (File == -1)
----------------
How about just calling truncate(..) method?

================
Comment at: lib/profile/InstrProfilingReduce.c:15
@@ +14,3 @@
+#include <mpi.h>
+COMPILER_RT_VISIBILITY int __llvm_write_prof_data = 1;
+COMPILER_RT_VISIBILITY int MPI_Finalize() {
----------------
Define this variable unconditionally in InstrProfiling.c file.

================
Comment at: lib/profile/InstrProfilingReduce.c:24
@@ +23,3 @@
+   if (Rank) {
+      MPI_Reduce(CountersBegin, NULL, CountersSize, MPI_UINT64_T, MPI_SUM,
+              0, MPI_COMM_WORLD);
----------------
Notice that value profile data is not 'reduced' here. It can be non-trivial to do so, but please add a comment about the limitation. 

================
Comment at: lib/profile/InstrProfilingReduce.c:30
@@ +29,3 @@
+              0, MPI_COMM_WORLD);
+   return PMPI_Finalize();
+}
----------------
Should return be used for Rank !=0 cases too?

================
Comment at: lib/profile/InstrProfilingStub.c:12
@@ +11,3 @@
+
+void MPI_Comm_rank(void *Comm, int *Rank) { abort(); }
+void MPI_Reduce() { abort(); }
----------------
guard these with #ifdef HAVE_MPI_H?

================
Comment at: lib/profile/InstrProfilingStub.c:14
@@ +13,2 @@
+void MPI_Reduce() { abort(); }
+void PMPI_Finalize() { abort(); }
----------------
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

================
Comment at: test/profile/instrprof-reduce.c:16
@@ +15,3 @@
+int PMPI_Finalize() { return 0; }
+int MPI_Finalize();
+
----------------
when HAVE_MPI_H is not defined, will this test case link? MPI_Finalize is not defined anywhere.


http://reviews.llvm.org/D18013