[PATCH] D62656: Make SwitchInstProfUpdateWrapper safer

[Yevgeny Rouban via Phabricator via llvm-commits]

yrouban planned changes to this revision.
yrouban marked an inline comment as done.
yrouban added a comment.

In D62656#1524574 <https://reviews.llvm.org/D62656#1524574>, @davidxl wrote:

> I suggest the following path:
>
> 1. add an option to enforce internal error on inconsistency; by default the inconsistency is ignored.


Ok. This option will probably simplify catching such bugs.



================
Comment at: llvm/unittests/IR/InstructionsTest.cpp:813
+    EXPECT_EQ(*SIW.getSuccessorWeight(2), 22u);
+    EXPECT_EQ(*SIW.getSuccessorWeight(3), 33u);
+  }
----------------
davidxl wrote:
> 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.
In future there will be no inconsistent states allowed and I suggest that the users write their code as if the stae is always consistent. During the transition period we just propagate the Undefined Behavior related to the prof data.



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

https://reviews.llvm.org/D62656