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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
llvm/lib/IR/ProfileSummary.cpp | ||
---|---|---|
180 | Does this need to be the 7th operand instead of 6? |
llvm/lib/IR/ProfileSummary.cpp | ||
---|---|---|
180 | (A small roundtrip test, eg. unittest/ProfileData/SampleProfTest.cpp, may be helpful to exercise getMD/getFromMD.) |
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
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.
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.
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? |
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? |
llvm/lib/IR/ProfileSummary.cpp | ||
---|---|---|
185 | Another good catch. Thanks. Fixed. |
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?