Make SwitchInstProfUpdateWrapper strict permanently

[yrouban]

Summary

We have been using -switch-inst-prof-update-wrapper-strict set to true by default for some time.
It is time to remove the safety stuff and make SwitchInstProfUpdateWrapper intolerant to inconsistencies in !prof branch_weights metadata of SwitchInst.
This patch gets rid of the Invalid state of SwitchInstProfUpdateWrapper and the option -switch-inst-prof-update-wrapper-strict.
So there is only two states: changed and unchanged.

[davidxl]

lgtm

llvm/trunk/include/llvm/IR/Instructions.h

public:
   }
 };
 
 /// A wrapper class to simplify modification of SwitchInst cases along with
 /// their prof branch_weights metadata.
 class SwitchInstProfUpdateWrapper {
   SwitchInst &SI;
   Optional<SmallVector<uint32_t, 8> > Weights = None;
-
-  // Sticky invalid state is needed to safely ignore operations with prof data
-  // in cases where SwitchInstProfUpdateWrapper is created from SwitchInst
-  // with inconsistent prof data. TODO: once we fix all prof data
-  // inconsistencies we can turn invalid state to assertions.
-  enum {
-    Invalid,
-    Initialized,
-    Changed
-  } State = Invalid;
+  bool Changed = false;
 
 protected:
   static MDNode *getProfBranchWeightsMD(const SwitchInst &SI);
 
   MDNode *buildProfBranchWeightsMD();
 
   void init();
 
 public:
   using CaseWeightOpt = Optional<uint32_t>;
   SwitchInst *operator->() { return &SI; }
   SwitchInst &operator*() { return SI; }
   operator SwitchInst *() { return &SI; }
 
   SwitchInstProfUpdateWrapper(SwitchInst &SI) : SI(SI) { init(); }
 
   ~SwitchInstProfUpdateWrapper() {
-    if (State == Changed)
+    if (Changed)
       SI.setMetadata(LLVMContext::MD_prof, buildProfBranchWeightsMD());
   }
 
   /// Delegate the call to the underlying SwitchInst::removeCase() and remove
   /// correspondent branch weight.
   SwitchInst::CaseIt removeCase(SwitchInst::CaseIt I);
 
   /// Delegate the call to the underlying SwitchInst::addCase() and set the

llvm/trunk/lib/IR/Instructions.cpp

 #include "llvm/Support/MathExtras.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <vector>
 
 using namespace llvm;
 
-static cl::opt<bool> SwitchInstProfUpdateWrapperStrict(
-    "switch-inst-prof-update-wrapper-strict", cl::Hidden,
-    cl::desc("Assert that prof branch_weights metadata is valid when creating "
-             "an instance of SwitchInstProfUpdateWrapper"),
-    cl::init(true));
-
 //===----------------------------------------------------------------------===//
 //                            AllocaInst Class
 //===----------------------------------------------------------------------===//
 
 Optional<uint64_t>
 AllocaInst::getAllocationSizeInBits(const DataLayout &DL) const {
   uint64_t Size = DL.getTypeAllocSizeInBits(getAllocatedType());
   if (isArrayAllocation()) {
SwitchInstProfUpdateWrapper::getProfBranchWeightsMD(const SwitchInst &SI) {
   if (MDNode *ProfileData = SI.getMetadata(LLVMContext::MD_prof))
     if (auto *MDName = dyn_cast<MDString>(ProfileData->getOperand(0)))
       if (MDName->getString() == "branch_weights")
         return ProfileData;
   return nullptr;
 }
 
 MDNode *SwitchInstProfUpdateWrapper::buildProfBranchWeightsMD() {
-  assert(State == Changed && "called only if metadata has changed");
+  assert(Changed && "called only if metadata has changed");
 
   if (!Weights)
     return nullptr;
 
   assert(SI.getNumSuccessors() == Weights->size() &&
          "num of prof branch_weights must accord with num of successors");
 
   bool AllZeroes =
       all_of(Weights.getValue(), [](uint32_t W) { return W == 0; });
 
   if (AllZeroes || Weights.getValue().size() < 2)
     return nullptr;
 
   return MDBuilder(SI.getParent()->getContext()).createBranchWeights(*Weights);
 }
 
 void SwitchInstProfUpdateWrapper::init() {
   MDNode *ProfileData = getProfBranchWeightsMD(SI);
-  if (!ProfileData) {
-    State = Initialized;
+  if (!ProfileData)
     return;
-  }
 
   if (ProfileData->getNumOperands() != SI.getNumSuccessors() + 1) {
-    State = Invalid;
-    if (SwitchInstProfUpdateWrapperStrict)
-      llvm_unreachable("number of prof branch_weights metadata operands does "
-                       "not correspond to number of succesors");
-    return;
   }
 
   SmallVector<uint32_t, 8> Weights;
   for (unsigned CI = 1, CE = SI.getNumSuccessors(); CI <= CE; ++CI) {
     ConstantInt *C = mdconst::extract<ConstantInt>(ProfileData->getOperand(CI));
     uint32_t CW = C->getValue().getZExtValue();
     Weights.push_back(CW);
   }
-  State = Initialized;
   this->Weights = std::move(Weights);
 }
 
 SwitchInst::CaseIt
 SwitchInstProfUpdateWrapper::removeCase(SwitchInst::CaseIt I) {
   if (Weights) {
     assert(SI.getNumSuccessors() == Weights->size() &&
            "num of prof branch_weights must accord with num of successors");
-    State = Changed;
+    Changed = true;
     // Copy the last case to the place of the removed one and shrink.
     // This is tightly coupled with the way SwitchInst::removeCase() removes
     // the cases in SwitchInst::removeCase(CaseIt).
     Weights.getValue()[I->getCaseIndex() + 1] = Weights.getValue().back();
     Weights.getValue().pop_back();
   }
   return SI.removeCase(I);
 }
 
 void SwitchInstProfUpdateWrapper::addCase(
     ConstantInt *OnVal, BasicBlock *Dest,
     SwitchInstProfUpdateWrapper::CaseWeightOpt W) {
   SI.addCase(OnVal, Dest);
 
-  if (State == Invalid)
-    return;
-
   if (!Weights && W && *W) {
-    State = Changed;
+    Changed = true;
     Weights = SmallVector<uint32_t, 8>(SI.getNumSuccessors(), 0);
     Weights.getValue()[SI.getNumSuccessors() - 1] = *W;
   } else if (Weights) {
-    State = Changed;
+    Changed = true;
     Weights.getValue().push_back(W ? *W : 0);
   }
   if (Weights)
     assert(SI.getNumSuccessors() == Weights->size() &&
            "num of prof branch_weights must accord with num of successors");
 }
 
 SymbolTableList<Instruction>::iterator
 SwitchInstProfUpdateWrapper::eraseFromParent() {
   // Instruction is erased. Mark as unchanged to not touch it in the destructor.
-  if (State != Invalid) {
-    State = Initialized;
-    if (Weights)
-      Weights->resize(0);
-  }
+  Changed = false;
   return SI.eraseFromParent();
 }
 
 SwitchInstProfUpdateWrapper::CaseWeightOpt
 SwitchInstProfUpdateWrapper::getSuccessorWeight(unsigned idx) {
   if (!Weights)
     return None;
   return Weights.getValue()[idx];
 }
 
 void SwitchInstProfUpdateWrapper::setSuccessorWeight(
     unsigned idx, SwitchInstProfUpdateWrapper::CaseWeightOpt W) {
-  if (!W || State == Invalid)
+  if (!W)
     return;
 
   if (!Weights && *W)
     Weights = SmallVector<uint32_t, 8>(SI.getNumSuccessors(), 0);
 
   if (Weights) {
     auto &OldW = Weights.getValue()[idx];
     if (*W != OldW) {
-      State = Changed;
+      Changed = true;
       OldW = *W;
     }
   }
 }
 
 SwitchInstProfUpdateWrapper::CaseWeightOpt
 SwitchInstProfUpdateWrapper::getSuccessorWeight(const SwitchInst &SI,
                                                 unsigned idx) {