Force check prof branch_weights consistency in SwitchInstProfUpdateWrapper

[yrouban]

Summary

This patch turns on the prof branch_weights metadata consistency check in SwitchInstProfUpdateWrapper.
If this patch causes a failure then please before reverting do report the IR that hits the assertion and try identifying the pass that introduces the inconsistency. We have to fix all such passes.
See also the upcoming change D61179 in the Verifier.

[davidxl]

What types of testing has been done ? I assume the code base now is relatively clean so it is safe to turn this on by default?

[yrouban]

In D64061#1566956, @davidxl wrote:

What types of testing has been done ? I assume the code base now is relatively clean so it is safe to turn this on by default?

Frankly speaking I'm not 100% sure that the code is safe. There could be projects that might hit the assertion. E.g I have to fix one pass internally at Azul. I tested LLVM monorepo make check-all and our Azul Zing test suits. So, I believe that the only way to find issues is to land this check D64061 and the verifyer D61179 one by one with some reasonable interval (one week?).

[davidxl]

let me do some internal PGO build tomorrow with the option on and get back.

[davidxl]

my test hit some other unrelated debug assert. Can you first change the debug assert into error message so that there is no need to use assert enabled build to test the option?

[yrouban]

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.

[davidxl]

please flip the flag to false by default and check in the rest first.

[yrouban]

extracted the preparation patch to the separate patch D64233.

[yrouban]

In D64061#1570873, @davidxl wrote:

please flip the flag to false by default and check in the rest first.

done. D64233 is landed.

[davidxl]

I did some test with the patch with samplePGO and it works fine. LGTM

llvm/lib/IR/Instructions.cpp

 #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(false));
+    cl::init(true));
 
 //===----------------------------------------------------------------------===//
 //                            AllocaInst Class
 //===----------------------------------------------------------------------===//
 
 Optional<uint64_t>
 AllocaInst::getAllocationSizeInBits(const DataLayout &DL) const {
   uint64_t Size = DL.getTypeAllocSizeInBits(getAllocatedType());

llvm/unittests/IR/InstructionsTest.cpp

TEST(InstructionsTest, SwitchInstProfUpdateWrapper) {
   }
 
   { // Create another wrapper and check that the data persist.
     SwitchInstProfUpdateWrapper SIW(*SI);
     EXPECT_EQ(*SIW.getSuccessorWeight(0), 99u);
     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) {
   SmallVector<int, 16> Indices({-1, 0, 7});
   ShuffleVectorInst::commuteShuffleMask(Indices, 4);
   EXPECT_THAT(Indices, testing::ContainerEq(ArrayRef<int>({-1, 4, 3})));
 }