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

[Wei Mi via Phabricator via llvm-commits]

wmi marked 4 inline comments as done.
wmi 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;
----------------
yamauchi wrote:
> 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?
The getVal will verify the field name before it read the value, but I add the getNumOperands check to make it more safe.


================
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",
----------------
yamauchi wrote:
> 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?
> 
> 
Good catch! Fixed.


================
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;
----------------
yamauchi wrote:
> Depending on whether/how we do tuple->getNumOperands check above, we may need to check if there's a 9th operand here.
> 
I did the check above.


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D78310