This is an archive of the discontinued LLVM Phabricator instance.

[ProfileSummary] Add partial profile annotation on IR.
ClosedPublic

Authored by wmi on Apr 16 2020, 10:50 AM.

Details

Summary

Profile and profile summary are usually read only once and then annotated on IR. The profile summary information on IR should include the value of the newly added partial profile flag, so that compilation phase like thinlto postlink can get the full set of profile information.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi created this revision.Apr 16 2020, 10:50 AM
hjyamauchi added inline comments.Apr 17 2020, 8:48 AM
llvm/lib/IR/ProfileSummary.cpp
180

Does this need to be the 7th operand instead of 6?

hjyamauchi added inline comments.Apr 17 2020, 9:01 AM
llvm/lib/IR/ProfileSummary.cpp
180

(A small roundtrip test, eg. unittest/ProfileData/SampleProfTest.cpp, may be helpful to exercise getMD/getFromMD.)

wmi marked 2 inline comments as done.Apr 18 2020, 10:25 AM

There are also a lot of tests which need to be updated because of the extra ProfileSummary MetaData component.

llvm/lib/IR/ProfileSummary.cpp
180

Fixed.

180

Good point. Done.

wmi updated this revision to Diff 258530.Apr 18 2020, 10:28 AM

Address Hiroshi's comment.

Update tests using the following script with a little manual tweaking for special cases.

  • update.sh ---------------------------------

file=$1
num=grep '!\"DetailedSummary\"' $file |sed 's%^[ ]*!\([0-9]*\).*%\1%g'

for ((i=200; $i>=$num; i--)); do

i_1=`expr $i '+' 1`
sed -i "s/!$i/!$i_1/g" $file

done
sed -i "/!\"DetailedSummary\"/i !$num = !{!\"IsPartialProfile\", i64 0}" $file
minus=expr $num '-' 1
plus=expr $num '+' 1

sed -i "s/!$minus, !$plus/!$minus, !$num, !$plus/g" $file

hjyamauchi accepted this revision.Apr 20 2020, 10:37 AM

Nice script.

Would it be also an option to make the IsPartialProfile MD node optional (by making getFromMD a bit more flexible and treating an absence as "0")? There may be a need to add more fields in the future.

Not a blocker, though.

LGTM.

This revision is now accepted and ready to land.Apr 20 2020, 10:37 AM
wmi added a comment.Apr 21 2020, 3:40 PM

Would it be also an option to make the IsPartialProfile MD node optional (by making getFromMD a bit more flexible and treating an absence as "0")? There may be a need to add more fields in the future.

Good suggestion. I was worrying that making some MD node optional may make getFromMD prone to wrong interpretation of fields, but I found getVal has already verified the name of the field.

So I change IsPartialProfile to be optional and I don't need the tests change anymore.

wmi updated this revision to Diff 259117.Apr 21 2020, 3:42 PM

Address Hiroshi's comment.

hjyamauchi added inline comments.Apr 22 2020, 10:17 AM
llvm/lib/IR/ProfileSummary.cpp
145

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?

164

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?

185

Depending on whether/how we do tuple->getNumOperands check above, we may need to check if there's a 9th operand here.

llvm/unittests/ProfileData/SampleProfTest.cpp
248

How about adding another version of this Summary -> MD -> Summary conversion test with the IsPartialProfile MD node dropped?

wmi marked 4 inline comments as done.Apr 23 2020, 10:12 AM
wmi added inline comments.
llvm/lib/IR/ProfileSummary.cpp
145

The getVal will verify the field name before it read the value, but I add the getNumOperands check to make it more safe.

164

Good catch! Fixed.

185

I did the check above.

llvm/unittests/ProfileData/SampleProfTest.cpp
248

test added.

wmi updated this revision to Diff 259619.Apr 23 2020, 10:13 AM

Address Hiroshi's comment.

hjyamauchi added inline comments.Apr 24 2020, 10:41 AM
llvm/lib/IR/ProfileSummary.cpp
185

Would we access out of bounds if we get an 8-entry tuple with the IsPartialProfile entry but without the DetailedSummary entry?

wmi marked an inline comment as done.Apr 24 2020, 12:06 PM
wmi added inline comments.
llvm/lib/IR/ProfileSummary.cpp
185

Another good catch. Thanks. Fixed.

wmi updated this revision to Diff 259951.Apr 24 2020, 12:12 PM

Address Hiroshi's comment.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2020, 8:35 AM