[PATCH] D78310: [ProfileSummary] Add partial profile annotation on IR.

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi added inline comments.


================
Comment at: llvm/lib/IR/ProfileSummary.cpp:148
   MDTuple *Tuple = dyn_cast_or_null<MDTuple>(MD);
-  if (!Tuple || Tuple->getNumOperands() != 8)
+  if (!Tuple)
     return nullptr;
----------------
Should we still retain the getNumOperands check (to be 8 or 9) here (or alternatively check the bounds right before each Tuple->getOperand call below) because we unconditionally access the first 8 fields below. eg. what happens if we unexpectedly get a 5-entry tuple here?


================
Comment at: llvm/lib/IR/ProfileSummary.cpp:167
   uint64_t NumCounts, TotalCount, NumFunctions, MaxFunctionCount, MaxCount,
-      MaxInternalCount;
-  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(1)), "TotalCount",
+      MaxInternalCount, IsPartialProfile;
+  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(i++)), "TotalCount",
----------------
Would we pass an uninitialized value in IsPartialProfile to the ProfileSummary constructor below if the IsPartialProfile MD node does not exist or is in the incorrect form?




================
Comment at: llvm/lib/IR/ProfileSummary.cpp:191
   SummaryEntryVector Summary;
-  if (!getSummaryFromMD(dyn_cast<MDTuple>(Tuple->getOperand(7)), Summary))
+  if (!getSummaryFromMD(dyn_cast<MDTuple>(Tuple->getOperand(i++)), Summary))
     return nullptr;
----------------
Depending on whether/how we do tuple->getNumOperands check above, we may need to check if there's a 9th operand here.



================
Comment at: llvm/unittests/ProfileData/SampleProfTest.cpp:248
 
     // Test that conversion of summary to and from Metadata works.
     Metadata *MD = Summary.getMD(Context);
----------------
How about adding another version of this Summary -> MD -> Summary conversion test with the IsPartialProfile MD node dropped?



Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78310/new/

https://reviews.llvm.org/D78310