[NFC] Use Optional<ProfileCount> to model invalid counts

[mtrofin]

Summary

ProfileCount could model invalid values, but a user had no indication
that the getCount method could return bogus data. Optional<ProfileCount>
addresses that, because the user must dereference the optional. In
addition, the patch removes concept duplication.

[dblaikie]

Generally like this sort of refactoring - though the const members are probably a fairly strong objection from me. The option to go further and make this a struct with optional helper member or non-member functions is less strenuous.

llvm/include/llvm/IR/Function.h

257–259

LLVM code doesn't generally use top-level const. Especially for members this makes things difficult - for instance you can't copy-assign objects with const members (so ProfileCount a, b; a = b; would be invalid) which usually isn't intended & can make things harder to work with.

261–266

Could this just be a struct at this point, maybe with an "isSynthetic" free function if testing "type == Synthetic" is too verbose?

The members are all both directly accessible, and can be initialized to any value, so the encapsulation doesn't seem to be providing anything?

llvm/lib/IR/Function.cpp

1927

separate patch could remove this else-after-return (& maybe add {} to the top-level if - it's a bit long to have no braces, even if it is only a single statement - or alternativeyl, invert that if statement and return early (if (!MD || !MD->getOperand(0)) return None; - oh, also could skip the getOperand checking and use dyn_cast_or_null on the next line instead))

[mtrofin]

llvm/include/llvm/IR/Function.h

257–259

Ack, I can do away with the const here, absence of setters achieves the same thing.

261–266

The value of encapsulation here is that it clarifies when the members are set: at construction only. Makes it so much easier (to me at least) to read the code by not forcing one to chase all references to a field and see whether it is set or read.

[mtrofin]

Removed const from the fields of ProfileCount

[dblaikie]

llvm/include/llvm/IR/Function.h

261–266

any piece of code could still change the values with: someCount = ProfileCount(a, b) - and it doesn't look like these values are used in particularly complicated ways, most have a scope of maybe 5 lines (which is great - and cases where they have long/complicated scopes more at risk of spooky-action-at-a-distance - refactoring to reduce the length of functions/scopes, pull out pieces into separate functions would be good)

This isn't done for most types in C++ because it runs up against C++ design idioms. Some codebases might use top-level const on local variables more often to enforce this & it's certainly where I'd go if I were suggesting addressing this issue - but it does add a lot of extra text/verbosity, and also isn't really done in the LLVM codebase (It's done in a smattering of places, but I think the inconsistency is also unhelpful - there's loads of values that could be changed so reducing scopes in general probably has more payoff across all those values)

[mtrofin]

llvm/include/llvm/IR/Function.h

261–266

But profileCount.Count = 5 means nothing - i.e. the point of the type is to encapsulate something coming from metadata. so what would be the usecase for freely setting the flags? Meaning, what is lost with the current design?

I get the not const-ing the fields for scenarios like re-assigning a local var. They aren't const anymore.

[mtrofin]

llvm/include/llvm/IR/Function.h

261–266

But profileCount.Count = 5 means nothing - i.e. the point of the type is to encapsulate something coming from metadata. so what would be the usecase for freely setting the flags? Meaning, what is lost with the current design?

I get the not const-ing the fields for scenarios like re-assigning a local var. They aren't const anymore.

Oh, maybe there's a misunderstanding: my argument was about not going to a naked struct, not about const fields.

[dblaikie]

Happy to approve the incremental improvement, even if it isn't as far as I'd go - up to you on the struct/class discussion.

llvm/include/llvm/IR/Function.h

261–266

(I think we're on the same page - discussing whether this could/should be a naked struct with two members, rather than having accessor member functions and private (albeit non-const) member variables)

Perhaps a reasonable summary of my perspective is: Since the values can be changed (by writing someCount = ProfileCount(a, b)) the read-only accessors don't provide the safety that a reader can assume the members are not modified - and we don't have that safety around most values/variables in C++ code, so I'm not sure why it's valuable to have it for this one type when all the other locals are modifiable (& this one is too, albeit slightly awkwardly by using assignment to the whole value rather than per-member).

The gain, to my mind, is a simpler API - like a function returning an int, we wouldn't usually wrap that in some other type that only provides read-access to it. This isn't much different.

It doesn't seem to me it provides the reader benefit you mentioned/motivated this design - that the reader doesn't have to worry about other mutations (because there can be mutations, by using the assignment) - and the scopes this exists in are quite small, so that reader benefit seems small and it's only one type/variable among many other mutable ones so I'm not sure it provides much value compared to reasoning about the rest of the variables/code in general.

[mtrofin]

llvm/include/llvm/IR/Function.h

261–266

I think there is value in design that communicates intent; it's less about hard guarantees (even const could be dropped with a cast). It's more about leaving indication that "this is a value, it's meant to be a pair, not really meant for free modification".

I think the tension is between simpler API vs usable (as in maintainable). I think leaving hints in the way the API is designed, while one may argue it complicates it, communicates intent better and makes use-as-intended more likely; so less bugs, etc.

For example, the previous ProfileCount design. One could argue it was simpler - you could just getCount(), no need to dereference an Optional - but that simplicity actually made the invalid state basically undiscoverable. (i.e. you had to read implementation details to get to that)

llvm/include/llvm/IR/Function.h

return static_cast<CallingConv::ID>((getSubclassDataFromValue() >> 4) &
                                         CallingConv::MaxID);
   }
   void setCallingConv(CallingConv::ID CC) {
     auto ID = static_cast<unsigned>(CC);
     assert(!(ID & ~CallingConv::MaxID) && "Unsupported calling convention");
     setValueSubclassData((getSubclassDataFromValue() & 0xc00f) | (ID << 4));
   }
 
-  enum ProfileCountType { PCT_Invalid, PCT_Real, PCT_Synthetic };
+  enum ProfileCountType { PCT_Real, PCT_Synthetic };
 
   /// Class to represent profile counts.
   ///
   /// This class represents both real and synthetic profile counts.
   class ProfileCount {
   private:
-    uint64_t Count;
-    ProfileCountType PCT;
-    static ProfileCount Invalid;
+    uint64_t Count = 0;
+    ProfileCountType PCT = PCT_Real;
 
   public:
-    ProfileCount() : Count(-1), PCT(PCT_Invalid) {}
     ProfileCount(uint64_t Count, ProfileCountType PCT)
         : Count(Count), PCT(PCT) {}
-    bool hasValue() const { return PCT != PCT_Invalid; }
     uint64_t getCount() const { return Count; }
     ProfileCountType getType() const { return PCT; }
     bool isSynthetic() const { return PCT == PCT_Synthetic; }
-    explicit operator bool() { return hasValue(); }
-    bool operator!() const { return !hasValue(); }
-    // Update the count retaining the same profile count type.
-    ProfileCount &setCount(uint64_t C) {
-      Count = C;
-      return *this;
-    }
-    static ProfileCount getInvalid() { return ProfileCount(-1, PCT_Invalid); }
   };
 
   /// Set the entry count for this function.
   ///
   /// Entry count is the number of times this function was executed based on
   /// pgo data. \p Imports points to a set of GUIDs that needs to
   /// be imported by the function for sample PGO, to enable the same inlines as
   /// the profiled optimized binary.
   void setEntryCount(ProfileCount Count,
                      const DenseSet<GlobalValue::GUID> *Imports = nullptr);
 
   /// A convenience wrapper for setting entry count
   void setEntryCount(uint64_t Count, ProfileCountType Type = PCT_Real,
                      const DenseSet<GlobalValue::GUID> *Imports = nullptr);
 
   /// Get the entry count for this function.
   ///
   /// Entry count is the number of times the function was executed.
   /// When AllowSynthetic is false, only pgo_data will be returned.
-  ProfileCount getEntryCount(bool AllowSynthetic = false) const;
+  Optional<ProfileCount> getEntryCount(bool AllowSynthetic = false) const;
 
   /// Return true if the function is annotated with profile data.
   ///
   /// Presence of entry counts from a profile run implies the function has
   /// profile annotations. If IncludeSynthetic is false, only return true
   /// when the profile data is real.
   bool hasProfileData(bool IncludeSynthetic = false) const {
     return getEntryCount(IncludeSynthetic).hasValue();

llvm/lib/Analysis/BlockFrequencyInfoImpl.cpp

 Optional<uint64_t>
 BlockFrequencyInfoImplBase::getProfileCountFromFreq(const Function &F,
                                                     uint64_t Freq,
                                                     bool AllowSynthetic) const {
   auto EntryCount = F.getEntryCount(AllowSynthetic);
   if (!EntryCount)
     return None;
   // Use 128 bit APInt to do the arithmetic to avoid overflow.
-  APInt BlockCount(128, EntryCount.getCount());
+  APInt BlockCount(128, EntryCount->getCount());
   APInt BlockFreq(128, Freq);
   APInt EntryFreq(128, getEntryFreq());
   BlockCount *= BlockFreq;
   // Rounded division of BlockCount by EntryFreq. Since EntryFreq is unsigned
   // lshr by 1 gives EntryFreq/2.
   BlockCount = (BlockCount + EntryFreq.lshr(1)).udiv(EntryFreq);
   return BlockCount.getLimitedValue();
 }

llvm/lib/Analysis/InlineCost.cpp

if (!CallerBFI)
       return false;
 
     // For now, limit to hot call site.
     if (!PSI->isHotCallSite(CandidateCall, CallerBFI))
       return false;
 
     // Make sure we have a nonzero entry count.
     auto EntryCount = F.getEntryCount();
-    if (!EntryCount || !EntryCount.getCount())
+    if (!EntryCount || !EntryCount->getCount())
       return false;
 
     BlockFrequencyInfo *CalleeBFI = &(GetBFI(F));
     if (!CalleeBFI)
       return false;
 
     return true;
   }
for (auto &BB : F) {
       auto ProfileCount = CalleeBFI->getBlockProfileCount(&BB);
       assert(ProfileCount.hasValue());
       CurrentSavings *= ProfileCount.getValue();
       CycleSavings += CurrentSavings;
     }
 
     // Compute the cycle savings per call.
     auto EntryProfileCount = F.getEntryCount();
-    assert(EntryProfileCount.hasValue() && EntryProfileCount.getCount());
-    auto EntryCount = EntryProfileCount.getCount();
+    assert(EntryProfileCount.hasValue() && EntryProfileCount->getCount());
+    auto EntryCount = EntryProfileCount->getCount();
     CycleSavings += EntryCount / 2;
     CycleSavings = CycleSavings.udiv(EntryCount);
 
     // Compute the total savings for the call site.
     auto *CallerBB = CandidateCall.getParent();
     BlockFrequencyInfo *CallerBFI = &(GetBFI(*(CallerBB->getParent())));
     CycleSavings += getCallsiteCost(this->CandidateCall, DL);
     CycleSavings *= CallerBFI->getBlockProfileCount(CallerBB).getValue();

llvm/lib/Analysis/ProfileSummaryInfo.cpp

 /// example, no profile data is available).
 bool ProfileSummaryInfo::isFunctionEntryHot(const Function *F) const {
   if (!F || !hasProfileSummary())
     return false;
   auto FunctionCount = F->getEntryCount();
   // FIXME: The heuristic used below for determining hotness is based on
   // preliminary SPEC tuning for inliner. This will eventually be a
   // convenience method that calls isHotCount.
-  return FunctionCount && isHotCount(FunctionCount.getCount());
+  return FunctionCount && isHotCount(FunctionCount->getCount());
 }
 
 /// Returns true if the function contains hot code. This can include a hot
 /// function entry count, hot basic block, or (in the case of Sample PGO)
 /// hot total call edge count.
 /// If it returns false, it either means it is not hot or it is unknown
 /// (for example, no profile data is available).
 bool ProfileSummaryInfo::isFunctionHotInCallGraph(
     const Function *F, BlockFrequencyInfo &BFI) const {
   if (!F || !hasProfileSummary())
     return false;
   if (auto FunctionCount = F->getEntryCount())
-    if (isHotCount(FunctionCount.getCount()))
+    if (isHotCount(FunctionCount->getCount()))
       return true;
 
   if (hasSampleProfile()) {
     uint64_t TotalCallCount = 0;
     for (const auto &BB : *F)
       for (const auto &I : BB)
         if (isa<CallInst>(I) || isa<InvokeInst>(I))
           if (auto CallCount = getProfileCount(cast<CallBase>(I), nullptr))

 /// the total call edge count is cold.
 /// If it returns false, it either means it is not cold or it is unknown
 /// (for example, no profile data is available).
 bool ProfileSummaryInfo::isFunctionColdInCallGraph(
     const Function *F, BlockFrequencyInfo &BFI) const {
   if (!F || !hasProfileSummary())
     return false;
   if (auto FunctionCount = F->getEntryCount())
-    if (!isColdCount(FunctionCount.getCount()))
+    if (!isColdCount(FunctionCount->getCount()))
       return false;
 
   if (hasSampleProfile()) {
     uint64_t TotalCallCount = 0;
     for (const auto &BB : *F)
       for (const auto &I : BB)
         if (isa<CallInst>(I) || isa<InvokeInst>(I))
           if (auto CallCount = getProfileCount(cast<CallBase>(I), nullptr))

 
 template <bool isHot>
 bool ProfileSummaryInfo::isFunctionHotOrColdInCallGraphNthPercentile(
     int PercentileCutoff, const Function *F, BlockFrequencyInfo &BFI) const {
   if (!F || !hasProfileSummary())
     return false;
   if (auto FunctionCount = F->getEntryCount()) {
     if (isHot &&
-        isHotCountNthPercentile(PercentileCutoff, FunctionCount.getCount()))
+        isHotCountNthPercentile(PercentileCutoff, FunctionCount->getCount()))
       return true;
     if (!isHot &&
-        !isColdCountNthPercentile(PercentileCutoff, FunctionCount.getCount()))
+        !isColdCountNthPercentile(PercentileCutoff, FunctionCount->getCount()))
       return false;
   }
   if (hasSampleProfile()) {
     uint64_t TotalCallCount = 0;
     for (const auto &BB : *F)
       for (const auto &I : BB)
         if (isa<CallInst>(I) || isa<InvokeInst>(I))
           if (auto CallCount = getProfileCount(cast<CallBase>(I), nullptr))
bool ProfileSummaryInfo::isFunctionEntryCold(const Function *F) const {
   if (F->hasFnAttribute(Attribute::Cold))
     return true;
   if (!hasProfileSummary())
     return false;
   auto FunctionCount = F->getEntryCount();
   // FIXME: The heuristic used below for determining coldness is based on
   // preliminary SPEC tuning for inliner. This will eventually be a
   // convenience method that calls isHotCount.
-  return FunctionCount && isColdCount(FunctionCount.getCount());
+  return FunctionCount && isColdCount(FunctionCount->getCount());
 }
 
 /// Compute the hot and cold thresholds.
 void ProfileSummaryInfo::computeThresholds() {
   auto &DetailedSummary = Summary->getDetailedSummary();
   auto &HotEntry = ProfileSummaryBuilder::getEntryForPercentile(
       DetailedSummary, ProfileSummaryCutoffHot);
   HotCountThreshold =

llvm/lib/CodeGen/MachineSizeOpts.cpp

 
 /// Like ProfileSummaryInfo::isFunctionColdInCallGraph but for
 /// MachineFunction.
 bool isFunctionColdInCallGraph(
     const MachineFunction *MF,
     ProfileSummaryInfo *PSI,
     const MachineBlockFrequencyInfo &MBFI) {
   if (auto FunctionCount = MF->getFunction().getEntryCount())
-    if (!PSI->isColdCount(FunctionCount.getCount()))
+    if (!PSI->isColdCount(FunctionCount->getCount()))
       return false;
   for (const auto &MBB : *MF)
     if (!isColdBlock(&MBB, PSI, &MBFI))
       return false;
   return true;
 }
 
 /// Like ProfileSummaryInfo::isFunctionHotInCallGraphNthPercentile but for
 /// MachineFunction.
 bool isFunctionHotInCallGraphNthPercentile(
     int PercentileCutoff,
     const MachineFunction *MF,
     ProfileSummaryInfo *PSI,
     const MachineBlockFrequencyInfo &MBFI) {
   if (auto FunctionCount = MF->getFunction().getEntryCount())
     if (PSI->isHotCountNthPercentile(PercentileCutoff,
-                                     FunctionCount.getCount()))
+                                     FunctionCount->getCount()))
       return true;
   for (const auto &MBB : *MF)
     if (isHotBlockNthPercentile(PercentileCutoff, &MBB, PSI, &MBFI))
       return true;
   return false;
 }
 
 bool isFunctionColdInCallGraphNthPercentile(
     int PercentileCutoff, const MachineFunction *MF, ProfileSummaryInfo *PSI,
     const MachineBlockFrequencyInfo &MBFI) {
   if (auto FunctionCount = MF->getFunction().getEntryCount())
     if (!PSI->isColdCountNthPercentile(PercentileCutoff,
-                                       FunctionCount.getCount()))
+                                       FunctionCount->getCount()))
       return false;
   for (const auto &MBB : *MF)
     if (!isColdBlockNthPercentile(PercentileCutoff, &MBB, PSI, &MBFI))
       return false;
   return true;
 }
 } // namespace machine_size_opts_detail
 

llvm/lib/IR/Function.cpp

void Function::setValueSubclassDataBit(unsigned Bit, bool On) {
   if (On)
     setValueSubclassData(getSubclassDataFromValue() | (1 << Bit));
   else
     setValueSubclassData(getSubclassDataFromValue() & ~(1 << Bit));
 }
 
 void Function::setEntryCount(ProfileCount Count,
                              const DenseSet<GlobalValue::GUID> *S) {
-  assert(Count.hasValue());
 #if !defined(NDEBUG)
   auto PrevCount = getEntryCount();
-  assert(!PrevCount.hasValue() || PrevCount.getType() == Count.getType());
+  assert(!PrevCount.hasValue() || PrevCount->getType() == Count.getType());
 #endif
 
   auto ImportGUIDs = getImportGUIDs();
   if (S == nullptr && ImportGUIDs.size())
     S = &ImportGUIDs;
 
   MDBuilder MDB(getContext());
   setMetadata(
       LLVMContext::MD_prof,
       MDB.createFunctionEntryCount(Count.getCount(), Count.isSynthetic(), S));
 }
 
 void Function::setEntryCount(uint64_t Count, Function::ProfileCountType Type,
                              const DenseSet<GlobalValue::GUID> *Imports) {
   setEntryCount(ProfileCount(Count, Type), Imports);
 }
 
-ProfileCount Function::getEntryCount(bool AllowSynthetic) const {
+Optional<ProfileCount> Function::getEntryCount(bool AllowSynthetic) const {
   MDNode *MD = getMetadata(LLVMContext::MD_prof);
   if (MD && MD->getOperand(0))
     if (MDString *MDS = dyn_cast<MDString>(MD->getOperand(0))) {
       if (MDS->getString().equals("function_entry_count")) {
         ConstantInt *CI = mdconst::extract<ConstantInt>(MD->getOperand(1));
         uint64_t Count = CI->getValue().getZExtValue();
         // A value of -1 is used for SamplePGO when there were no samples.
         // Treat this the same as unknown.
         if (Count == (uint64_t)-1)
-          return ProfileCount::getInvalid();
+          return None;
         return ProfileCount(Count, PCT_Real);
       } else if (AllowSynthetic &&
                  MDS->getString().equals("synthetic_function_entry_count")) {
         ConstantInt *CI = mdconst::extract<ConstantInt>(MD->getOperand(1));
         uint64_t Count = CI->getValue().getZExtValue();
         return ProfileCount(Count, PCT_Synthetic);
       }
     }
-  return ProfileCount::getInvalid();
+  return None;
 }
 
 DenseSet<GlobalValue::GUID> Function::getImportGUIDs() const {
   DenseSet<GlobalValue::GUID> R;
   if (MDNode *MD = getMetadata(LLVMContext::MD_prof))
     if (MDString *MDS = dyn_cast<MDString>(MD->getOperand(0)))
       if (MDS->getString().equals("function_entry_count"))
         for (unsigned i = 2; i < MD->getNumOperands(); i++)

llvm/lib/Transforms/IPO/PartialInlining.cpp

std::vector<User *> Users(Cloner.ClonedFunc->user_begin(),
                             Cloner.ClonedFunc->user_end());
 
   DenseMap<User *, uint64_t> CallSiteToProfCountMap;
   auto CalleeEntryCount = Cloner.OrigFunc->getEntryCount();
   if (CalleeEntryCount)
     computeCallsiteToProfCountMap(Cloner.ClonedFunc, CallSiteToProfCountMap);
 
   uint64_t CalleeEntryCountV =
-      (CalleeEntryCount ? CalleeEntryCount.getCount() : 0);
+      (CalleeEntryCount ? CalleeEntryCount->getCount() : 0);
 
   bool AnyInline = false;
   for (User *User : Users) {
     CallBase *CB = getSupportedCallBase(User);
 
     if (isLimitReached())
       continue;
 
if (Cloner.ClonedOI)
       NumPartialInlined++;
     else
       NumColdOutlinePartialInlined++;
   }
 
   if (AnyInline) {
     Cloner.IsFunctionInlined = true;
     if (CalleeEntryCount)
-      Cloner.OrigFunc->setEntryCount(
-          CalleeEntryCount.setCount(CalleeEntryCountV));
+      Cloner.OrigFunc->setEntryCount(Function::ProfileCount(
+          CalleeEntryCountV, CalleeEntryCount->getType()));
     OptimizationRemarkEmitter OrigFuncORE(Cloner.OrigFunc);
     OrigFuncORE.emit([&]() {
       return OptimizationRemark(DEBUG_TYPE, "PartiallyInlined", Cloner.OrigFunc)
              << "Partially inlined into at least one caller";
     });
   }
 
   return AnyInline;

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

 // Using the ratio b/w sums of profile count values and BFI count values to
 // adjust the func entry count.
 static void fixFuncEntryCount(PGOUseFunc &Func, LoopInfo &LI,
                               BranchProbabilityInfo &NBPI) {
   Function &F = Func.getFunc();
   BlockFrequencyInfo NBFI(F, NBPI, LI);
 #ifndef NDEBUG
   auto BFIEntryCount = F.getEntryCount();
-  assert(BFIEntryCount.hasValue() && (BFIEntryCount.getCount() > 0) &&
+  assert(BFIEntryCount.hasValue() && (BFIEntryCount->getCount() > 0) &&
          "Invalid BFI Entrycount");
 #endif
   auto SumCount = APFloat::getZero(APFloat::IEEEdouble());
   auto SumBFICount = APFloat::getZero(APFloat::IEEEdouble());
   for (auto &BBI : F) {
     uint64_t CountValue = 0;
     uint64_t BFICountValue = 0;
     if (!Func.findBBInfo(&BBI))

llvm/lib/Transforms/Utils/InlineFunction.cpp

CallerBFI->setBlockFreqAndScale(
       ClonedBBs);
 }
 
 /// Update the branch metadata for cloned call instructions.
 static void updateCallProfile(Function *Callee, const ValueToValueMapTy &VMap,
                               const ProfileCount &CalleeEntryCount,
                               const CallBase &TheCall, ProfileSummaryInfo *PSI,
                               BlockFrequencyInfo *CallerBFI) {
-  if (!CalleeEntryCount.hasValue() || CalleeEntryCount.isSynthetic() ||
-      CalleeEntryCount.getCount() < 1)
+  if (CalleeEntryCount.isSynthetic() || CalleeEntryCount.getCount() < 1)
     return;
   auto CallSiteCount = PSI ? PSI->getProfileCount(TheCall, CallerBFI) : None;
   int64_t CallCount =
       std::min(CallSiteCount.getValueOr(0), CalleeEntryCount.getCount());
   updateProfileCallee(Callee, -CallCount, &VMap);
 }
 
 void llvm::updateProfileCallee(
     Function *Callee, int64_t EntryDelta,
     const ValueMap<const Value *, WeakTrackingVH> *VMap) {
   auto CalleeCount = Callee->getEntryCount();
   if (!CalleeCount.hasValue())
     return;
 
-  const uint64_t PriorEntryCount = CalleeCount.getCount();
+  const uint64_t PriorEntryCount = CalleeCount->getCount();
 
   // Since CallSiteCount is an estimate, it could exceed the original callee
   // count and has to be set to 0 so guard against underflow.
   const uint64_t NewEntryCount =
       (EntryDelta < 0 && static_cast<uint64_t>(-EntryDelta) > PriorEntryCount)
           ? 0
           : PriorEntryCount + EntryDelta;
 
Function::iterator FirstNewBlock;
     // subtracted from callee profile, and the inlined clone also doesn't need
     // to be scaled based on call site count.
     if (IFI.UpdateProfile) {
       if (IFI.CallerBFI != nullptr && IFI.CalleeBFI != nullptr)
         // Update the BFI of blocks cloned into the caller.
         updateCallerBFI(OrigBB, VMap, IFI.CallerBFI, IFI.CalleeBFI,
                         CalledFunc->front());
 
-      updateCallProfile(CalledFunc, VMap, CalledFunc->getEntryCount(), CB,
-                        IFI.PSI, IFI.CallerBFI);
+      if (auto Profile = CalledFunc->getEntryCount())
+        updateCallProfile(CalledFunc, VMap, *Profile, CB, IFI.PSI,
+                          IFI.CallerBFI);
     }
 
     // Inject byval arguments initialization.
     for (ByValInit &Init : ByValInits)
       HandleByValArgumentInit(Init.Ty, Init.Dst, Init.Src, Caller->getParent(),
                               &*FirstNewBlock, IFI);
 
     Optional<OperandBundleUse> ParentDeopt =

llvm/unittests/IR/MetadataTest.cpp

TEST_F(FunctionAttachmentTest, Verifier) {
   EXPECT_FALSE(verifyFunction(*F));
 
   // So can definitions.
   (void)new UnreachableInst(Context, BasicBlock::Create(Context, "bb", F));
   EXPECT_FALSE(verifyModule(*F->getParent()));
   EXPECT_FALSE(verifyFunction(*F));
 }
 
-TEST_F(FunctionAttachmentTest, EntryCount) {
+TEST_F(FunctionAttachmentTest, RealEntryCount) {
   Function *F = getFunction("foo");
   EXPECT_FALSE(F->getEntryCount().hasValue());
   F->setEntryCount(12304, Function::PCT_Real);
   auto Count = F->getEntryCount();
   EXPECT_TRUE(Count.hasValue());
-  EXPECT_EQ(12304u, Count.getCount());
-  EXPECT_EQ(Function::PCT_Real, Count.getType());
+  EXPECT_EQ(12304u, Count->getCount());
+  EXPECT_EQ(Function::PCT_Real, Count->getType());
+}
 
-  // Repeat the same for synthetic counts.
-  F = getFunction("bar");
+TEST_F(FunctionAttachmentTest, SyntheticEntryCount) {
+  Function *F = getFunction("bar");
   EXPECT_FALSE(F->getEntryCount().hasValue());
   F->setEntryCount(123, Function::PCT_Synthetic);
-  Count = F->getEntryCount(true /*allow synthetic*/);
+  auto Count = F->getEntryCount(true /*allow synthetic*/);
   EXPECT_TRUE(Count.hasValue());
-  EXPECT_EQ(123u, Count.getCount());
-  EXPECT_EQ(Function::PCT_Synthetic, Count.getType());
+  EXPECT_EQ(123u, Count->getCount());
+  EXPECT_EQ(Function::PCT_Synthetic, Count->getType());
 }
 
 TEST_F(FunctionAttachmentTest, SubprogramAttachment) {
   Function *F = getFunction("foo");
   DISubprogram *SP = getSubprogram();
   F->setSubprogram(SP);
 
   // Note that the static_cast confirms that F->getSubprogram() actually