[PATCH] D62656: Make SwitchInstProfUpdateWrapper safer

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

I suggest the following path:

1. add an option to enforce internal error on inconsistency; by default the inconsistency is ignored.
2. fix the exposed root cause one by one
3. turn on the option by default



================
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
----------------
yrouban wrote:
> 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.
What I meant is that this kind of interface can be source of inconsistency. 


================
Comment at: llvm/unittests/IR/InstructionsTest.cpp:813
+    EXPECT_EQ(*SIW.getSuccessorWeight(2), 22u);
+    EXPECT_EQ(*SIW.getSuccessorWeight(3), 33u);
+  }
----------------
yrouban wrote:
> 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.
Such interface behavior is not desirable. It is certainly surprising to the user which calls addCase as there is no way to know that the weight is ignored.


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

https://reviews.llvm.org/D62656