[ProfileSummary] Add partial profile annotation on IR.

[wmi]

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.

[hjyamauchi]

llvm/lib/IR/ProfileSummary.cpp

190

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

[hjyamauchi]

llvm/lib/IR/ProfileSummary.cpp

190

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

[wmi]

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

llvm/lib/IR/ProfileSummary.cpp

190

Fixed.

190

Good point. Done.

[wmi]

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]

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.

[wmi]

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]

Address Hiroshi's comment.

[hjyamauchi]

llvm/lib/IR/ProfileSummary.cpp

153

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?

173

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?

198

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

277–278

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

[wmi]

llvm/lib/IR/ProfileSummary.cpp

153

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

173

Good catch! Fixed.

198

I did the check above.

llvm/unittests/ProfileData/SampleProfTest.cpp

277–278

test added.

[wmi]

Address Hiroshi's comment.

[hjyamauchi]

llvm/lib/IR/ProfileSummary.cpp

198

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

[wmi]

llvm/lib/IR/ProfileSummary.cpp

198

Another good catch. Thanks. Fixed.

[wmi]

Address Hiroshi's comment.

[hjyamauchi]

LGTM.

llvm/include/llvm/IR/ProfileSummary.h

private:
   Metadata *getDetailedSummaryMD(LLVMContext &Context);
 
 public:
   static const int Scale = 1000000;
 
   ProfileSummary(Kind K, SummaryEntryVector DetailedSummary,
                  uint64_t TotalCount, uint64_t MaxCount,
                  uint64_t MaxInternalCount, uint64_t MaxFunctionCount,
-                 uint32_t NumCounts, uint32_t NumFunctions)
+                 uint32_t NumCounts, uint32_t NumFunctions,
+                 bool Partial = false)
       : PSK(K), DetailedSummary(std::move(DetailedSummary)),
         TotalCount(TotalCount), MaxCount(MaxCount),
         MaxInternalCount(MaxInternalCount), MaxFunctionCount(MaxFunctionCount),
-        NumCounts(NumCounts), NumFunctions(NumFunctions) {}
+        NumCounts(NumCounts), NumFunctions(NumFunctions), Partial(Partial) {}
 
   Kind getKind() const { return PSK; }
   /// Return summary information as metadata.
-  Metadata *getMD(LLVMContext &Context);
+  Metadata *getMD(LLVMContext &Context, bool AddPartialField = true);
   /// Construct profile summary from metdata.
   static ProfileSummary *getFromMD(Metadata *MD);
   SummaryEntryVector &getDetailedSummary() { return DetailedSummary; }
   uint32_t getNumFunctions() { return NumFunctions; }
   uint64_t getMaxFunctionCount() { return MaxFunctionCount; }
   uint32_t getNumCounts() { return NumCounts; }
   uint64_t getTotalCount() { return TotalCount; }
   uint64_t getMaxCount() { return MaxCount; }
   uint64_t getMaxInternalCount() { return MaxInternalCount; }
   void setPartialProfile(bool PP) { Partial = PP; }
   bool isPartialProfile() { return Partial; }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_IR_PROFILESUMMARY_H

llvm/lib/IR/ProfileSummary.cpp

Metadata *ProfileSummary::getDetailedSummaryMD(LLVMContext &Context) {
   return MDTuple::get(Context, Ops);
 }
 
 // This returns an MDTuple representing this ProfileSummary object. The first
 // entry of this tuple is another MDTuple of two elements: a string
 // "ProfileFormat" and a string representing the format ("InstrProf" or
 // "SampleProfile"). The rest of the elements of the outer MDTuple are specific
 // to the kind of profile summary as returned by getFormatSpecificMD.
-Metadata *ProfileSummary::getMD(LLVMContext &Context) {
+// IsPartialProfile is an optional field and \p AddPartialField will decide
+// whether to add a field for it.
+Metadata *ProfileSummary::getMD(LLVMContext &Context, bool AddPartialField) {
   const char *KindStr[3] = {"InstrProf", "CSInstrProf", "SampleProfile"};
-  Metadata *Components[] = {
-    getKeyValMD(Context, "ProfileFormat", KindStr[PSK]),
-    getKeyValMD(Context, "TotalCount", getTotalCount()),
-    getKeyValMD(Context, "MaxCount", getMaxCount()),
-    getKeyValMD(Context, "MaxInternalCount", getMaxInternalCount()),
-    getKeyValMD(Context, "MaxFunctionCount", getMaxFunctionCount()),
-    getKeyValMD(Context, "NumCounts", getNumCounts()),
-    getKeyValMD(Context, "NumFunctions", getNumFunctions()),
-    getDetailedSummaryMD(Context),
-  };
+  SmallVector<Metadata *, 16> Components;
+  Components.push_back(getKeyValMD(Context, "ProfileFormat", KindStr[PSK]));
+  Components.push_back(getKeyValMD(Context, "TotalCount", getTotalCount()));
+  Components.push_back(getKeyValMD(Context, "MaxCount", getMaxCount()));
+  Components.push_back(
+      getKeyValMD(Context, "MaxInternalCount", getMaxInternalCount()));
+  Components.push_back(
+      getKeyValMD(Context, "MaxFunctionCount", getMaxFunctionCount()));
+  Components.push_back(getKeyValMD(Context, "NumCounts", getNumCounts()));
+  Components.push_back(getKeyValMD(Context, "NumFunctions", getNumFunctions()));
+  if (AddPartialField)
+    Components.push_back(
+        getKeyValMD(Context, "IsPartialProfile", isPartialProfile()));
+  Components.push_back(getDetailedSummaryMD(Context));
   return MDTuple::get(Context, Components);
 }
 
 // Parse an MDTuple representing (Key, Val) pair.
 static bool getVal(MDTuple *MD, const char *Key, uint64_t &Val) {
   if (!MD)
     return false;
   if (MD->getNumOperands() != 2)
     return false;
   MDString *KeyMD = dyn_cast<MDString>(MD->getOperand(0));
   ConstantAsMetadata *ValMD = dyn_cast<ConstantAsMetadata>(MD->getOperand(1));
   if (!KeyMD || !ValMD)
     return false;
   if (!KeyMD->getString().equals(Key))
     return false;
   Val = cast<ConstantInt>(ValMD->getValue())->getZExtValue();
   return true;
 }
 
-// Check if an MDTuple represents a (Key, Val) pair.
-static bool isKeyValuePair(MDTuple *MD, const char *Key, const char *Val) {
+// Check if an MDTuple represents a (Key, Val) pair. If \p SkipVal is true,
+// it will only check whether MDTuple contains key identical to \p Key.
+static bool isKeyValuePair(MDTuple *MD, const char *Key, const char *Val,
+                           bool SkipVal = false) {
   if (!MD || MD->getNumOperands() != 2)
     return false;
   MDString *KeyMD = dyn_cast<MDString>(MD->getOperand(0));
   MDString *ValMD = dyn_cast<MDString>(MD->getOperand(1));
-  if (!KeyMD || !ValMD)
+  if (!KeyMD || (!SkipVal && !ValMD))
     return false;
-  if (!KeyMD->getString().equals(Key) || !ValMD->getString().equals(Val))
+  if (!KeyMD->getString().equals(Key) ||
+      (!SkipVal && !ValMD->getString().equals(Val)))
     return false;
   return true;
 }
 
 // Parse an MDTuple representing detailed summary.
 static bool getSummaryFromMD(MDTuple *MD, SummaryEntryVector &Summary) {
   if (!MD || MD->getNumOperands() != 2)
     return false;
Summary.emplace_back(cast<ConstantInt>(Op0->getValue())->getZExtValue(),
                          cast<ConstantInt>(Op1->getValue())->getZExtValue(),
                          cast<ConstantInt>(Op2->getValue())->getZExtValue());
   }
   return true;
 }
 
 ProfileSummary *ProfileSummary::getFromMD(Metadata *MD) {
   MDTuple *Tuple = dyn_cast_or_null<MDTuple>(MD);
-  if (!Tuple || Tuple->getNumOperands() != 8)
+  if (!Tuple && (Tuple->getNumOperands() < 8 || Tuple->getNumOperands() > 9))
     return nullptr;
 
-  auto &FormatMD = Tuple->getOperand(0);
+  int i = 0;
+  auto &FormatMD = Tuple->getOperand(i++);
   ProfileSummary::Kind SummaryKind;
   if (isKeyValuePair(dyn_cast_or_null<MDTuple>(FormatMD), "ProfileFormat",
                      "SampleProfile"))
     SummaryKind = PSK_Sample;
   else if (isKeyValuePair(dyn_cast_or_null<MDTuple>(FormatMD), "ProfileFormat",
                           "InstrProf"))
     SummaryKind = PSK_Instr;
   else if (isKeyValuePair(dyn_cast_or_null<MDTuple>(FormatMD), "ProfileFormat",
                           "CSInstrProf"))
     SummaryKind = PSK_CSInstr;
   else
     return nullptr;
 
   uint64_t NumCounts, TotalCount, NumFunctions, MaxFunctionCount, MaxCount,
       MaxInternalCount;
-  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(1)), "TotalCount",
+  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(i++)), "TotalCount",
               TotalCount))
     return nullptr;
-  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(2)), "MaxCount", MaxCount))
+  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(i++)), "MaxCount", MaxCount))
     return nullptr;
-  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(3)), "MaxInternalCount",
+  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(i++)), "MaxInternalCount",
               MaxInternalCount))
     return nullptr;
-  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(4)), "MaxFunctionCount",
+  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(i++)), "MaxFunctionCount",
               MaxFunctionCount))
     return nullptr;
-  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(5)), "NumCounts", NumCounts))
+  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(i++)), "NumCounts",
+              NumCounts))
     return nullptr;
-  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(6)), "NumFunctions",
+  if (!getVal(dyn_cast<MDTuple>(Tuple->getOperand(i++)), "NumFunctions",
               NumFunctions))
     return nullptr;
+  // Initialize IsPartialProfile because the field is optional.
+  uint64_t IsPartialProfile = 0;
+  if (isKeyValuePair(dyn_cast<MDTuple>(Tuple->getOperand(i)),
+                     "IsPartialProfile", nullptr, true))
+    getVal(dyn_cast<MDTuple>(Tuple->getOperand(i++)), "IsPartialProfile",
+           IsPartialProfile);
 
   SummaryEntryVector Summary;
-  if (!getSummaryFromMD(dyn_cast<MDTuple>(Tuple->getOperand(7)), Summary))
+  if (!getSummaryFromMD(dyn_cast<MDTuple>(Tuple->getOperand(i++)), Summary))
     return nullptr;
   return new ProfileSummary(SummaryKind, std::move(Summary), TotalCount,
                             MaxCount, MaxInternalCount, MaxFunctionCount,
-                            NumCounts, NumFunctions);
+                            NumCounts, NumFunctions, IsPartialProfile);
 }

llvm/test/Transforms/PGOProfile/cspgo_profile_summary.ll

 ; CSPGOSUMMARY: {{![0-9]+}} = !{i32 1, !"ProfileSummary", !1}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"ProfileFormat", !"InstrProf"}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"TotalCount", i64 2100001}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"MaxCount", i64 800000}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"MaxInternalCount", i64 399999}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"MaxFunctionCount", i64 800000}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"NumCounts", i64 14}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"NumFunctions", i64 8}
-; CSPGOSUMMARY: {{![0-9]+}} = !{!"DetailedSummary", !10}
+; CSPGOSUMMARY: {{![0-9]+}} = !{!"DetailedSummary", !{{[0-9]+}}}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{i32 1, !"CSProfileSummary", !{{[0-9]+}}}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"ProfileFormat", !"CSInstrProf"}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"TotalCount", i64 1299950}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"MaxCount", i64 200000}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"MaxInternalCount", i64 100000}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"MaxFunctionCount", i64 200000}
 ; CSPGOSUMMARY: {{![0-9]+}} = !{!"NumCounts", i64 23}
 ; CSPGOSUMMARY-DAG: ![[BW_CSPGO_BAR]] = !{!"branch_weights", i32 100000, i32 100000}
 ; CSPGOSUMMARY-DAG: ![[BW1_CSPGO_FOO]] = !{!"branch_weights", i32 100000, i32 0}
 ; CSPGOSUMMARY-DAG: ![[BW2_CSPGO_FOO]] = !{!"branch_weights", i32 0, i32 100000}

llvm/test/Transforms/PGOProfile/unreachable_bb.ll

entry:
   unreachable
 return:
   ret void
 }
 
 declare void @bar()
 
 ;USE: !0 = !{i32 1, !"ProfileSummary", !1}
-;USE: !1 = !{!2, !3, !4, !5, !6, !7, !8, !9}
+;USE: !1 = !{!2, !3, !4, !5, !6, !7, !8, !9, !10}
 ;USE: !2 = !{!"ProfileFormat", !"InstrProf"}
 ;USE: !3 = !{!"TotalCount", i64 0}
 
 

llvm/unittests/ProfileData/SampleProfTest.cpp

*OS << R"(
       # Types 'int' and 'long' are equivalent
       type i l
       # Function names 'foo' and 'faux' are equivalent
       name 3foo 4faux
     )";
     OS->close();
   }
 
+  // Verify profile summary is consistent in the roundtrip to and from
+  // Metadata. \p AddPartialField is to choose whether the Metadata
+  // contains the IsPartialProfile field which is optional.
+  void verifyProfileSummary(ProfileSummary &Summary, Module &M,
+                            const bool AddPartialField) {
+    LLVMContext &Context = M.getContext();
+    const bool IsPartialProfile = Summary.isPartialProfile();
+    auto VerifySummary = [IsPartialProfile](ProfileSummary &Summary) mutable {
+      ASSERT_EQ(ProfileSummary::PSK_Sample, Summary.getKind());
+      ASSERT_EQ(137392u, Summary.getTotalCount());
+      ASSERT_EQ(8u, Summary.getNumCounts());
+      ASSERT_EQ(4u, Summary.getNumFunctions());
+      ASSERT_EQ(1437u, Summary.getMaxFunctionCount());
+      ASSERT_EQ(60351u, Summary.getMaxCount());
+      ASSERT_EQ(IsPartialProfile, Summary.isPartialProfile());
+
+      uint32_t Cutoff = 800000;
+      auto Predicate = [&Cutoff](const ProfileSummaryEntry &PE) {
+        return PE.Cutoff == Cutoff;
+      };
+      std::vector<ProfileSummaryEntry> &Details = Summary.getDetailedSummary();
+      auto EightyPerc = find_if(Details, Predicate);
+      Cutoff = 900000;
+      auto NinetyPerc = find_if(Details, Predicate);
+      Cutoff = 950000;
+      auto NinetyFivePerc = find_if(Details, Predicate);
+      Cutoff = 990000;
+      auto NinetyNinePerc = find_if(Details, Predicate);
+      ASSERT_EQ(60000u, EightyPerc->MinCount);
+      ASSERT_EQ(12557u, NinetyPerc->MinCount);
+      ASSERT_EQ(12557u, NinetyFivePerc->MinCount);
+      ASSERT_EQ(610u, NinetyNinePerc->MinCount);
+    };
+    VerifySummary(Summary);
+
+    // Test that conversion of summary to and from Metadata works.
+    Metadata *MD = Summary.getMD(Context, AddPartialField);
+    ASSERT_TRUE(MD);
+    ProfileSummary *PS = ProfileSummary::getFromMD(MD);
+    ASSERT_TRUE(PS);
+    VerifySummary(*PS);
+    delete PS;
+
+    // Test that summary can be attached to and read back from module.
+    PS = ProfileSummary::getFromMD(MD);
+
+    M.eraseNamedMetadata(M.getOrInsertModuleFlagsMetadata());
+    M.setProfileSummary(MD, ProfileSummary::PSK_Sample);
+    MD = M.getProfileSummary(/* IsCS */ false);
+    ASSERT_TRUE(MD);
+    PS = ProfileSummary::getFromMD(MD);
+    ASSERT_TRUE(PS);
+    VerifySummary(*PS);
+    delete PS;
+  }
+
   void testRoundTrip(SampleProfileFormat Format, bool Remap, bool UseMD5) {
     SmallVector<char, 128> ProfilePath;
     ASSERT_TRUE(NoError(llvm::sys::fs::createTemporaryFile("profile", "", ProfilePath)));
     StringRef Profile(ProfilePath.data(), ProfilePath.size());
     createWriter(Format, Profile);
     if (Format == SampleProfileFormat::SPF_Ext_Binary && UseMD5)
       static_cast<SampleProfileWriterExtBinary *>(Writer.get())->setUseMD5();
 
void testRoundTrip(SampleProfileFormat Format, bool Remap, bool UseMD5) {
     StringRef MconstructRep =
         getRepInFormat(MconstructName, UseMD5, MconstructGUID);
     std::string StringviewGUID;
     StringRef StringviewRep =
         getRepInFormat(StringviewName, UseMD5, StringviewGUID);
     ASSERT_EQ(1000u, CTMap.get()[MconstructRep]);
     ASSERT_EQ(437u, CTMap.get()[StringviewRep]);
 
-    auto VerifySummary = [](ProfileSummary &Summary) mutable {
-      ASSERT_EQ(ProfileSummary::PSK_Sample, Summary.getKind());
-      ASSERT_EQ(137392u, Summary.getTotalCount());
-      ASSERT_EQ(8u, Summary.getNumCounts());
-      ASSERT_EQ(4u, Summary.getNumFunctions());
-      ASSERT_EQ(1437u, Summary.getMaxFunctionCount());
-      ASSERT_EQ(60351u, Summary.getMaxCount());
-
-      uint32_t Cutoff = 800000;
-      auto Predicate = [&Cutoff](const ProfileSummaryEntry &PE) {
-        return PE.Cutoff == Cutoff;
-      };
-      std::vector<ProfileSummaryEntry> &Details = Summary.getDetailedSummary();
-      auto EightyPerc = find_if(Details, Predicate);
-      Cutoff = 900000;
-      auto NinetyPerc = find_if(Details, Predicate);
-      Cutoff = 950000;
-      auto NinetyFivePerc = find_if(Details, Predicate);
-      Cutoff = 990000;
-      auto NinetyNinePerc = find_if(Details, Predicate);
-      ASSERT_EQ(60000u, EightyPerc->MinCount);
-      ASSERT_EQ(12557u, NinetyPerc->MinCount);
-      ASSERT_EQ(12557u, NinetyFivePerc->MinCount);
-      ASSERT_EQ(610u, NinetyNinePerc->MinCount);
-    };
 
     ProfileSummary &Summary = Reader->getSummary();
-    VerifySummary(Summary);
+    Summary.setPartialProfile(true);
+    verifyProfileSummary(Summary, M, true);
 
-    // Test that conversion of summary to and from Metadata works.
-    Metadata *MD = Summary.getMD(Context);
-    ASSERT_TRUE(MD);
-    ProfileSummary *PS = ProfileSummary::getFromMD(MD);
-    ASSERT_TRUE(PS);
-    VerifySummary(*PS);
-    delete PS;
+    Summary.setPartialProfile(false);
+    verifyProfileSummary(Summary, M, true);
 
-    // Test that summary can be attached to and read back from module.
-    M.setProfileSummary(MD, ProfileSummary::PSK_Sample);
-    MD = M.getProfileSummary(/* IsCS */ false);
-    ASSERT_TRUE(MD);
-    PS = ProfileSummary::getFromMD(MD);
-    ASSERT_TRUE(PS);
-    VerifySummary(*PS);
-    delete PS;
+    Summary.setPartialProfile(false);
+    verifyProfileSummary(Summary, M, false);
   }
 
   void addFunctionSamples(StringMap<FunctionSamples> *Smap, const char *Fname,
                           uint64_t TotalSamples, uint64_t HeadSamples) {
     StringRef Name(Fname);
     FunctionSamples FcnSamples;
     FcnSamples.setName(Name);
     FcnSamples.addTotalSamples(TotalSamples);