[llvm][misexpect] Avoid division by 0 when using sample profiling

[paulkirth]

Summary

MisExpect diagnostics should not prevent compilation from succeeding, and the
assertion is insufficient to prevent division by zero in release builds.

This patch addresses that by replacing the assert with an early return.

Additionally, it disables MisExpect diagnostics when using sample profiling,
since this is the only known case where this error has manifested.

[paulkirth]

One concern I have here, since someone reported this to me is that perhaps some of MisExpect's assumptions regarding when branch weight metadata will be attached is erroneous.

The key assumptions we make are that for frontend instrumentation that any branch weights that have been attached when LowerExpectIntrinsics runs will originate from profiling. The other key assumption we make is that when branch weights are assigned for IR instrumentation that LowerExpectIntrinsic is the only place that can add these weights before they are added by the IR or sample profiles. If that's not the case, then we need to discuss a way to address that shortcoming.

That concern doesn't change the need for this fix, but may require some more in depth discussion on discourse about how to address this in a comprehensive manner.

[tejohnson]

In D124302#3469029, @paulkirth wrote:

One concern I have here, since someone reported this to me is that perhaps some of MisExpect's assumptions regarding when branch weight metadata will be attached is erroneous.

The key assumptions we make are that for frontend instrumentation that any branch weights that have been attached when LowerExpectIntrinsics runs will originate from profiling. The other key assumption we make is that when branch weights are assigned for IR instrumentation that LowerExpectIntrinsic is the only place that can add these weights before they are added by the IR or sample profiles. If that's not the case, then we need to discuss a way to address that shortcoming.

That concern doesn't change the need for this fix, but may require some more in depth discussion on discourse about how to address this in a comprehensive manner.

The failing test case was with SamplePGO. I went back and looked at the original patch and realized that it doesn't seem to have any test cases for SamplePGO, even though it is being called there. It probably makes sense to look more closely at the IR in that case to see how this code is getting the 0 "expected" edge weights. Rather than this change, how about disabling misexpect verification for sample PGO as a short term fix, then doing some investigation and adding a fix along with sample PGO tests when re-enabling it there?

[paulkirth]

In D124302#3469104, @tejohnson wrote:

The failing test case was with SamplePGO. I went back and looked at the original patch and realized that it doesn't seem to have any test cases for SamplePGO, even though it is being called there. It probably makes sense to look more closely at the IR in that case to see how this code is getting the 0 "expected" edge weights. Rather than this change, how about disabling misexpect verification for sample PGO as a short term fix, then doing some investigation and adding a fix along with sample PGO tests when re-enabling it there?

That seems like a reasonable choice. I'm a bit apprehensive about relying on the assert to prevent this type of error though. Maybe we do both for now?

[paulkirth]

Disable diagnostics when using sample profiling

[paulkirth]

fix typo in comment

[tejohnson]

lgtm

Sure, that seems fine to me. I would add a FIXME to add back the assert as well. There are a few typos in the patch description, can you fix those before submitting?

[paulkirth]

Add FIXME comment

llvm/lib/Transforms/IPO/SampleProfile.cpp

 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/ProfiledCallGraph.h"
 #include "llvm/Transforms/IPO/SampleContextTracker.h"
 #include "llvm/Transforms/IPO/SampleProfileProbe.h"
 #include "llvm/Transforms/Instrumentation.h"
 #include "llvm/Transforms/Utils/CallPromotionUtils.h"
 #include "llvm/Transforms/Utils/Cloning.h"
-#include "llvm/Transforms/Utils/MisExpect.h"
 #include "llvm/Transforms/Utils/SampleProfileInference.h"
 #include "llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h"
 #include "llvm/Transforms/Utils/SampleProfileLoaderBaseUtil.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <functional>
 #include <limits>
for (unsigned I = 0; I < TI->getNumSuccessors(); ++I) {
       if (Weight != 0) {
         if (Weight > MaxWeight) {
           MaxWeight = Weight;
           MaxDestInst = Succ->getFirstNonPHIOrDbgOrLifetime();
         }
       }
     }
 
-    misexpect::checkExpectAnnotations(*TI, Weights, /*IsFrontend=*/false);
+    // FIXME: Re-enable for sample profiling after investigating why the sum
+    // of branch weights can be 0
+    //
+    // misexpect::checkExpectAnnotations(*TI, Weights, /*IsFrontend=*/false);
 
     uint64_t TempWeight;
     // Only set weights if there is at least one non-zero weight.
     // In any other case, let the analyzer set weights.
     // Do not set weights if the weights are present unless under
     // OverwriteExistingWeights. In ThinLTO, the profile annotation is done
     // twice. If the first annotation already set the weights, the second pass
     // does not need to set it. With OverwriteExistingWeights, Blocks with zero

llvm/lib/Transforms/Utils/MisExpect.cpp

void verifyMisExpect(Instruction &I, ArrayRef<uint32_t> RealWeights,
   const uint64_t RealWeightsTotal =
       std::accumulate(RealWeights.begin(), RealWeights.end(), (uint64_t)0,
                       std::plus<uint64_t>());
   const uint64_t NumUnlikelyTargets = RealWeights.size() - 1;
 
   uint64_t TotalBranchWeight =
       LikelyBranchWeight + (UnlikelyBranchWeight * NumUnlikelyTargets);
 
-  assert((TotalBranchWeight >= LikelyBranchWeight) && (TotalBranchWeight > 0) &&
-         "TotalBranchWeight is less than the Likely branch weight");
+  // FIXME: When we've addressed sample profiling, restore the assertion
+  //
+  // We cannot calculate branch probability if either of these invariants aren't
+  // met. However, MisExpect diagnostics should not prevent code from compiling,
+  // so we simply forgo emitting diagnostics here, and return early.
+  if ((TotalBranchWeight == 0) || (TotalBranchWeight <= LikelyBranchWeight))
+    return;
 
   // To determine our threshold value we need to obtain the branch probability
   // for the weights added by llvm.expect and use that proportion to calculate
   // our threshold based on the collected profile data.
   auto LikelyProbablilty = BranchProbability::getBranchProbability(
       LikelyBranchWeight, TotalBranchWeight);
 
   uint64_t ScaledThreshold = LikelyProbablilty.scale(RealWeightsTotal);