[PATCH] D15258: [PGO] Remove data races on Data->Values field

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/profile/InstrProfilingValue.c:159
@@ -184,4 +158,3 @@
   for (I = (__llvm_profile_data *)DataBegin; I != DataEnd; ++I) {
-
     DEF_VALUE_RECORD(R, I->NumValueSites, I->Values);
 
----------------
davidxl wrote:
> betulb wrote:
> > As an overall comment, I'm not too fond of using macros this heavily. Specific to this line, DEF_VALUE_RECORD is used only once and there is a macro definition for its contents. It's not necessary. Secondly, R is passed to the macro w/o being declared in this context. I'd rather have preferred that R is declared first and then passed to the macro (if macro usage is ever a requisite).
> yes, after the cleanup, this one can be removed -- as it is not reused anywhere else.
For the record, this macro was also introduced to 'mimic' C++ where variable declaration and construction are done together.

================
Comment at: lib/profile/InstrProfilingValue.c:181
@@ -237,1 +180,3 @@
+
+  return S;
 }
----------------
betulb wrote:
> Is there any need to return S in this implementation? We may also remove the need to pass the ValueDataSize around.
good point.  It was needed to indicate if there is vp data to be written -- but the rewrite makes it not necessary. Will remove.

================
Comment at: lib/profile/InstrProfilingWriter.c:68
@@ -63,5 +67,3 @@
     return -1;
-  if (ValueDataBegin) {
-    ProfDataIOVec IOVec2[] = {{ValueDataBegin, sizeof(char), ValueDataSize}};
-    if (Writer(IOVec2, sizeof(IOVec2) / sizeof(*IOVec2), &WriterCtx))
-      return -1;
+  if (ValueDataSize) {
+    ValueProfData **ValueDataArray = ValueDataBegin;
----------------
betulb wrote:
> Instead of ValueDataSize, why not check for if ValueDataBegin is null?
yes. Will change.


http://reviews.llvm.org/D15258