[SampleProfile] [ICP] Handle the case when the option `MaxNumPromotions` is zero.

[mingmingl]

Summary

In places where MaxNumPromotions is used to allocated an array, bail out early to prevent allocating an array of length 0.

[mingmingl]

+Rong and Wenlei based on history but let me know if there are suggested reviewers!

Also if it makes sense to add a test I'd be glad to!

[MaskRay]

Is there a test triggering the zero-length code path? It's valid to allocate an array of length zero. The code just cannot dereference the pointer.

[mingmingl]

Is there a test triggering the zero-length code path?

The context is that, I try to set sample-profile-icp-max-prom to zero (with the goal of evaluating how much ICP helps), and the compilation hangs without completion (and triggers timeout in crosstool)

By bootstrapping compiler w/ this patch, the build completed (I didn't verify the generated binary though)

btw I cc'ed you in an internal email with pointers to artifacts.

It's valid to allocate an array of length zero. The code just cannot dereference the pointer.

[1] said it's not ok for c / c++ to allocate a fixed size array of size 0 yet [2] mentions that allocating is ok but de-referencing is not..

It seems to me that the exact support differs from compiler to complier though. I'm not fluent in identifying to the relevant implementations, but maybe adding logs in the locally-boostrapped compiler helps (in the internal context)

[1] https://stackoverflow.com/questions/9722632/what-happens-if-i-define-a-0-size-array-in-c-c/9723093
[2] https://stackoverflow.com/questions/1087042/c-new-int0-will-it-allocate-memory

[davidxl]

Is it possible to check this in the caller context?

[xur]

I think the reason MaxNumPromotion==0 breaks is likely the 0 size array think (like MaskRay mentioned).

Explicitly checking of 0 is a good fix here.

LGTM except I'm not sure if the second check is needed.

llvm/lib/Transforms/IPO/SampleProfile.cpp

846

Why this is needed? This is update the profile counter after we do the promotion.
If the check works in doesHistoryAllowICP(), MaxNumPromptions here should not be 0, right?

[mingmingl]

Move zero value check into the tryPromoteAndInlineCandidate (cleaner and saves symbol look-up) . Meanwhile keep the check in updateIDTMetadata since it has two callers, and the other caller (generateMDProfMetadata) executes as long as the function is changed (e.g., by inliner without icp).

[mingmingl]

In D120295#3338540, @davidxl wrote:

Is it possible to check this in the caller context?

Done.

In D120295#3338813, @xur wrote:

I think the reason MaxNumPromotion==0 breaks is likely the 0 size array think (like MaskRay mentioned).

Explicitly checking of 0 is a good fix here.

LGTM except I'm not sure if the second check is needed.

Yeah I realized doesHistoryAllowICP will return false, and updateIDTMetadata won't be executed within tryPromoteAndInlineCandidate.

There is another caller of updateIDTMetadata (https://github.com/llvm/llvm-project/blob/606cb8548a1b7763e0c8489c5efe66803a7ede72/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1640), so keep the value check within updateIDTMetadata.

[davidxl]

lgtm

[mingmingl]

fix a compile error.

llvm/lib/Transforms/IPO/SampleProfile.cpp

 /// Usually \p Sum is the sum of counts of all the targets for \p Inst.
 /// If it is 0, it means updateIDTMetaData is used to mark a
 /// certain target to be promoted already. If it is not zero,
 /// we expect to use it to update the total count in the value profile.
 static void
 updateIDTMetaData(Instruction &Inst,
                   const SmallVectorImpl<InstrProfValueData> &CallTargets,
                   uint64_t Sum) {
+  // Bail out early if MaxNumPromotions is zero.
+  // This prevents allocating an array of zero length below.
+  //
+  // Note `updateIDTMetaData` is called in two places so check
+  // `MaxNumPromotions` inside it.
+  if (MaxNumPromotions == 0)
+    return;
   uint32_t NumVals = 0;
   // OldSum is the existing total count in the value profile data.
   uint64_t OldSum = 0;
   std::unique_ptr<InstrProfValueData[]> ValueData =
       std::make_unique<InstrProfValueData[]>(MaxNumPromotions);
   bool Valid =
       getValueProfDataFromInst(Inst, IPVK_IndirectCallTarget, MaxNumPromotions,
                                ValueData.get(), NumVals, OldSum, true);

 ///                   promoting given candidate.
 /// \param Sum        Prorated sum of remaining target counts for indirect call
 ///                   after promoting given candidate.
 /// \param InlinedCallSite  Output vector for new call sites exposed after
 /// inlining.
 bool SampleProfileLoader::tryPromoteAndInlineCandidate(
     Function &F, InlineCandidate &Candidate, uint64_t SumOrigin, uint64_t &Sum,
     SmallVector<CallBase *, 8> *InlinedCallSite) {
+  // Bail out early if MaxNumPromotions is zero.
+  // This prevents allocating an array of zero length in callees below.
+  if (MaxNumPromotions == 0)
+    return false;
   auto CalleeFunctionName = Candidate.CalleeSamples->getFuncName();
   auto R = SymbolMap.find(CalleeFunctionName);
   if (R == SymbolMap.end() || !R->getValue())
     return false;
 
   auto &CI = *Candidate.CallInstr;
   if (!doesHistoryAllowICP(CI, R->getValue()->getName()))
     return false;