From 8be236c9f9d3cf3fdd9406eb705abecc70a2da1e Mon Sep 17 00:00:00 2001 From: Yevgeny Rouban Date: Thu, 12 Sep 2019 03:41:34 +0000 Subject: [PATCH] Make SwitchInstProfUpdateWrapper strict permanently 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. Reviewers: davidx, nikic, eraman, reames, chandlerc Reviewed By: davidx Differential Revision: https://reviews.llvm.org/D67435 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@371707 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/IR/Instructions.h | 13 ++--------- lib/IR/Instructions.cpp | 41 ++++++++++------------------------ 2 files changed, 14 insertions(+), 40 deletions(-) diff --git a/include/llvm/IR/Instructions.h b/include/llvm/IR/Instructions.h index 009ec25a65a..c99fd784263 100644 --- a/include/llvm/IR/Instructions.h +++ b/include/llvm/IR/Instructions.h @@ -3459,16 +3459,7 @@ public: class SwitchInstProfUpdateWrapper { SwitchInst &SI; Optional > 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); @@ -3486,7 +3477,7 @@ public: SwitchInstProfUpdateWrapper(SwitchInst &SI) : SI(SI) { init(); } ~SwitchInstProfUpdateWrapper() { - if (State == Changed) + if (Changed) SI.setMetadata(LLVMContext::MD_prof, buildProfBranchWeightsMD()); } diff --git a/lib/IR/Instructions.cpp b/lib/IR/Instructions.cpp index 18d7719ecb3..37d3371e233 100644 --- a/lib/IR/Instructions.cpp +++ b/lib/IR/Instructions.cpp @@ -45,12 +45,6 @@ using namespace llvm; -static cl::opt 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 //===----------------------------------------------------------------------===// @@ -3897,7 +3891,7 @@ SwitchInstProfUpdateWrapper::getProfBranchWeightsMD(const SwitchInst &SI) { } MDNode *SwitchInstProfUpdateWrapper::buildProfBranchWeightsMD() { - assert(State == Changed && "called only if metadata has changed"); + assert(Changed && "called only if metadata has changed"); if (!Weights) return nullptr; @@ -3916,17 +3910,12 @@ MDNode *SwitchInstProfUpdateWrapper::buildProfBranchWeightsMD() { 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; + llvm_unreachable("number of prof branch_weights metadata operands does " + "not correspond to number of succesors"); } SmallVector Weights; @@ -3935,7 +3924,6 @@ void SwitchInstProfUpdateWrapper::init() { uint32_t CW = C->getValue().getZExtValue(); Weights.push_back(CW); } - State = Initialized; this->Weights = std::move(Weights); } @@ -3944,7 +3932,7 @@ 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). @@ -3959,15 +3947,12 @@ void SwitchInstProfUpdateWrapper::addCase( SwitchInstProfUpdateWrapper::CaseWeightOpt W) { SI.addCase(OnVal, Dest); - if (State == Invalid) - return; - if (!Weights && W && *W) { - State = Changed; + Changed = true; Weights = SmallVector(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) @@ -3978,11 +3963,9 @@ void SwitchInstProfUpdateWrapper::addCase( SymbolTableList::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; + if (Weights) + Weights->resize(0); return SI.eraseFromParent(); } @@ -3995,7 +3978,7 @@ SwitchInstProfUpdateWrapper::getSuccessorWeight(unsigned idx) { void SwitchInstProfUpdateWrapper::setSuccessorWeight( unsigned idx, SwitchInstProfUpdateWrapper::CaseWeightOpt W) { - if (!W || State == Invalid) + if (!W) return; if (!Weights && *W) @@ -4004,7 +3987,7 @@ void SwitchInstProfUpdateWrapper::setSuccessorWeight( if (Weights) { auto &OldW = Weights.getValue()[idx]; if (*W != OldW) { - State = Changed; + Changed = true; OldW = *W; } } -- 2.40.0