Page MenuHomePhabricator

Make SwitchInstProfUpdateWrapper safer
ClosedPublic

Authored by yrouban on May 30 2019, 6:00 AM.

Details

Summary

While prof branch_weights inconsistencies are being fixed patch by patch (pass by pass) we need SwitchInstProfUpdateWrapper to be safe with respect to inconsistent metadata that can come from passes that have not been fixed yet. See the bug found by @nikic in D62126.
This patch introduces one more state (called Invalid) to the wrapper class that allows users to work with the underlying SwitchInst ignoring the prof metadata changes.
Created a unit test for the SwitchInstProfUpdateWrapper class.

Diff Detail

Repository
rL LLVM

Event Timeline

yrouban created this revision.May 30 2019, 6:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 6:00 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
davidxl added inline comments.May 30 2019, 10:59 AM
llvm/lib/IR/Instructions.cpp
3907 ↗(On Diff #202166)

should we fix the underlying problem that leads to this inconsistent state?

yrouban marked an inline comment as done.May 30 2019, 11:08 AM
yrouban added inline comments.
llvm/lib/IR/Instructions.cpp
3907 ↗(On Diff #202166)

That is what I'm trying to do.
The problem could have been solved by one of the proposed patches. See the dependency graph.
It would be great to identify where the problem stems from.

What I meant is that we should never see inconsistent state and instead of setting invalid state, we assert there. Ideally, the profile setting/update APIs should help user avoid creating the inconsistent state in the first place (i.e., catch the problem on the spot).

llvm/unittests/IR/InstructionsTest.cpp
797 ↗(On Diff #202166)

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.

813 ↗(On Diff #202166)

why it is not 39?

xbolva00 added inline comments.
llvm/include/llvm/IR/Instructions.h
3444 ↗(On Diff #202166)

Typo

What I meant is that we should never see inconsistent state and instead of setting invalid state, we assert there.

Yes. But in LLVM there are many places to fix. So we have to fix all of them at once and set the assertion here. The other option is to fix them one by one and tolerate such inconsistencies.
For example, SimplifyCFG is safe: it checks that the prof data is ok and only then makes its prof changes.

yrouban marked 3 inline comments as done.May 30 2019, 9:04 PM
yrouban added inline comments.
llvm/unittests/IR/InstructionsTest.cpp
797 ↗(On Diff #202166)

it is just a test and SI is just a SwitchInst.

813 ↗(On Diff #202166)

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.

yrouban updated this revision to Diff 202363.May 30 2019, 9:06 PM

Formatted and fixed one typo.

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
llvm/unittests/IR/InstructionsTest.cpp
797 ↗(On Diff #202166)

What I meant is that this kind of interface can be source of inconsistency.

813 ↗(On Diff #202166)

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.

@davidxl I believe the changes here are only temporary until D61179 lands, which will enforce this at the verifier level. As such I don't think there's much value in introducing a verification option here, because this is not really the right place to do that.

What I would suggest though is to maybe drop the branch weight metadata entirely if it's not valid. I think it's pretty weird (as was commented above) that you might end up with "valid" metadata after performing additional transformations (removing/adding cases), but that metadata will just be an accidental assignment, more or less. I think it would be better to discard it wholesale than risk an incorrect reinterpretation.

yrouban planned changes to this revision.May 31 2019, 12:05 AM
yrouban marked an inline comment as done.

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.

llvm/unittests/IR/InstructionsTest.cpp
813 ↗(On Diff #202166)

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.

yrouban updated this revision to Diff 202393.May 31 2019, 2:09 AM

added option switch-inst-prof-update-wrapper-strict to assert if prof data is inconsistent. It is off by default.
Probably it makes sense to enable prof branch_weights validation (patch D61179) in the same way.

once the cleanup is done, the state tracking should be removed as well.

llvm/lib/IR/Instructions.cpp
48 ↗(On Diff #202393)

typo: Strinct

asbirlea added inline comments.May 31 2019, 1:22 PM
llvm/lib/IR/Instructions.cpp
48 ↗(On Diff #202393)

also typo: Swicth, var name and in the cl::desc below.

yrouban updated this revision to Diff 202644.Jun 2 2019, 10:02 PM
yrouban marked 2 inline comments as done.

fixed typos

davidxl accepted this revision.Jun 3 2019, 8:59 AM

lgtm assuming the state tracking code gets removed (or put under NDEBUG in some way) after the cleanup.

This revision is now accepted and ready to land.Jun 3 2019, 8:59 AM
This revision was automatically updated to reflect the committed changes.