Page MenuHomePhabricator

[NFC] SimplifyCFG prof branch_weighs handling is simplified
ClosedPublic

Authored by yrouban on May 20 2019, 12:46 AM.

Details

Summary

Using the new SwitchInst handler introduced in D62122 this patch simplifies 3 places of prof branch_weights handling.

Diff Detail

Repository
rL LLVM

Event Timeline

yrouban created this revision.May 20 2019, 12:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 12:46 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
davidxl added inline comments.May 20 2019, 12:05 PM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
3632 ↗(On Diff #200208)

Perhaps add a 'SplitDefault' method in SwitchInstProfBranchWeightsWrapper class?

4438 ↗(On Diff #200208)

Early return instead of doing the guard here?

yrouban updated this revision to Diff 200411.May 21 2019, 12:02 AM
yrouban marked 2 inline comments as done.

Done early return in eliminateDeadSwitchCases as suggested.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
3632 ↗(On Diff #200208)

I think it does not look like a frequently used routine which is worth a separate method.

reames accepted this revision.Jun 12 2019, 3:22 PM

LGTM w/two required changes. (If you want to just make the changes, you can submit without further review. )

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
867 ↗(On Diff #200411)

SwitchInstProfBranchWeightsWrapper SI(*cast<SwitchInst>(TI));

(i.e. avoid a default init and copy construct)

879 ↗(On Diff #200411)

This HasWeight check doesn't appear to have a corresponding bit of code inside the wrapper class. In particular, this means that the old code handles malformed metadata and the new one does not.

I know you want to disallow this case, but for the moment, as a practical migration, please add the bailout inside the wrapper. That would make this change obviously NFC (which it isn't right now.)

This revision is now accepted and ready to land.Jun 12 2019, 3:22 PM
yrouban marked an inline comment as done.Jun 14 2019, 5:13 AM
yrouban added inline comments.
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
879 ↗(On Diff #200411)

HasWeight is equivalent to the condition SI.State == Invalid.
SI.State is checked in every method of the wrapper, particularly in the constructor, in SI.removeCase() and in the destructor. Those correspond to the if (HasWeight) places.

Again, LGTM.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
879 ↗(On Diff #200411)

Yep, I see it now. I must have been looking at a stale checkout or something when I commented originally. Feel free to ignore this thread.

This revision was automatically updated to reflect the committed changes.