[PATCH] D62656: Make SwitchInstProfUpdateWrapper safer

[Yevgeny Rouban via Phabricator via llvm-commits]

yrouban marked 3 inline comments as done.
yrouban added inline comments.


================
Comment at: llvm/unittests/IR/InstructionsTest.cpp:797
+  // Make prof data invalid by adding one extra weight.
+  SI->setMetadata(LLVMContext::MD_prof,
+                  MDBuilder(C).createBranchWeights({ 99, 11, 22, 33})); // extra
----------------
davidxl wrote:
> looks like exposing the raw interface to setMetadata for SI is not safe. Using wrapper interface can help catch error that leads to inconsistent state.
it is just a test and SI is just a SwitchInst.


================
Comment at: llvm/unittests/IR/InstructionsTest.cpp:813
+    EXPECT_EQ(*SIW.getSuccessorWeight(2), 22u);
+    EXPECT_EQ(*SIW.getSuccessorWeight(3), 33u);
+  }
----------------
davidxl wrote:
> why it is not 39?
That is because 39 was set when SIW was inconsistent. So //SwitchInstProfUpdateWrapper::addCase()// skipped setting the weight 39 and delegated addCase() to the underlying SwitchInst. The prof data attached to the underlying SwicthInst was kept intact and had 4 operands: 99, 11, 22, 33. So the next time we created a new wrapper it started with valid state.


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

https://reviews.llvm.org/D62656