[PATCH] D64061: Force check prof branch_weights consistency in SwitchInstProfUpdateWrapper

[Yevgeny Rouban via Phabricator via llvm-commits]

yrouban updated this revision to Diff 207968.
yrouban added a comment.

Changed assert(false) to llvm_unreachable() for release builds to fail on inconsistencies. Though it seems to be too strict as the code can tolerate such inconsistencies.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64061/new/

https://reviews.llvm.org/D64061

Files:
  llvm/lib/IR/Instructions.cpp
  llvm/unittests/IR/InstructionsTest.cpp


Index: llvm/unittests/IR/InstructionsTest.cpp
===================================================================
--- llvm/unittests/IR/InstructionsTest.cpp
+++ llvm/unittests/IR/InstructionsTest.cpp
@@ -792,44 +792,6 @@
     EXPECT_EQ(*SIW.getSuccessorWeight(1), 11u);
     EXPECT_EQ(*SIW.getSuccessorWeight(2), 22u);
   }
-
-  // Make prof data invalid by adding one extra weight.
-  SI->setMetadata(LLVMContext::MD_prof, MDBuilder(C).createBranchWeights(
-                                            { 99, 11, 22, 33 })); // extra
-  { // Invalid prof data makes wrapper act as if there were no prof data.
-    SwitchInstProfUpdateWrapper SIW(*SI);
-    ASSERT_FALSE(SIW.getSuccessorWeight(0).hasValue());
-    ASSERT_FALSE(SIW.getSuccessorWeight(1).hasValue());
-    ASSERT_FALSE(SIW.getSuccessorWeight(2).hasValue());
-    SIW.addCase(ConstantInt::get(Int32Ty, 3), BB3.get(), 39);
-    ASSERT_FALSE(SIW.getSuccessorWeight(3).hasValue()); // did not add weight 39
-  }
-
-  { // With added 3rd case the prof data become consistent with num of cases.
-    SwitchInstProfUpdateWrapper SIW(*SI);
-    EXPECT_EQ(*SIW.getSuccessorWeight(0), 99u);
-    EXPECT_EQ(*SIW.getSuccessorWeight(1), 11u);
-    EXPECT_EQ(*SIW.getSuccessorWeight(2), 22u);
-    EXPECT_EQ(*SIW.getSuccessorWeight(3), 33u);
-  }
-
-  // Make prof data invalid by removing one extra weight.
-  SI->setMetadata(LLVMContext::MD_prof,
-                  MDBuilder(C).createBranchWeights({ 99, 11, 22 })); // shorter
-  { // Invalid prof data makes wrapper act as if there were no prof data.
-    SwitchInstProfUpdateWrapper SIW(*SI);
-    ASSERT_FALSE(SIW.getSuccessorWeight(0).hasValue());
-    ASSERT_FALSE(SIW.getSuccessorWeight(1).hasValue());
-    ASSERT_FALSE(SIW.getSuccessorWeight(2).hasValue());
-    SIW.removeCase(SwitchInst::CaseIt(SI, 2));
-  }
-
-  { // With removed 3rd case the prof data become consistent with num of cases.
-    SwitchInstProfUpdateWrapper SIW(*SI);
-    EXPECT_EQ(*SIW.getSuccessorWeight(0), 99u);
-    EXPECT_EQ(*SIW.getSuccessorWeight(1), 11u);
-    EXPECT_EQ(*SIW.getSuccessorWeight(2), 22u);
-  }
 }
 
 TEST(InstructionsTest, CommuteShuffleMask) {
Index: llvm/lib/IR/Instructions.cpp
===================================================================
--- llvm/lib/IR/Instructions.cpp
+++ llvm/lib/IR/Instructions.cpp
@@ -49,7 +49,7 @@
     "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(false));
+    cl::init(true));
 
 //===----------------------------------------------------------------------===//
 //                            AllocaInst Class
@@ -3913,9 +3913,8 @@
   if (ProfileData->getNumOperands() != SI.getNumSuccessors() + 1) {
     State = Invalid;
     if (SwitchInstProfUpdateWrapperStrict)
-      assert(false &&
-             "number of prof branch_weights metadata operands corresponds to"
-             " number of succesors");
+      llvm_unreachable("number of prof branch_weights metadata operands does "
+                       "not correspond to number of succesors");
     return;
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D64061.207968.patch
Type: text/x-patch
Size: 3182 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190704/e2c914a8/attachment.bin>