Page MenuHomePhabricator

[NFC][InlineCost] cleanup - comments, overflow handling.
ClosedPublic

Authored by fedor.sergeev on Apr 15 2019, 10:33 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

fedor.sergeev created this revision.Apr 15 2019, 10:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2019, 10:33 PM
Herald added a subscriber: haicheng. · View Herald Transcript
yrouban added inline comments.Apr 18 2019, 12:38 AM
lib/Analysis/InlineCost.cpp
247 ↗(On Diff #195297)

I believe that both Inc and UpperBound must be of the same type as Cost (i.e. int) but the parameters of std::max() should be extended.

apilipenko accepted this revision.Apr 26 2019, 6:14 PM
apilipenko added inline comments.
lib/Analysis/InlineCost.cpp
247 ↗(On Diff #195297)

Agree. An explicit cast to a wider type is more clear.

This revision is now accepted and ready to land.Apr 26 2019, 6:14 PM
fedor.sergeev marked an inline comment as done.Apr 27 2019, 3:35 AM
fedor.sergeev added inline comments.
lib/Analysis/InlineCost.cpp
247 ↗(On Diff #195297)

int64_t came from cost calculations in CallAnalyzer::visitSwitchInst, which specifically uses uint64_t in many places, both for increment and upper bound.
Practically speaking, reaching INTMAX there is hardly possible since that would reqiure having, say, more than INTMAX/5 Cases in a single switch.
However I would like to avoid doing semantical changes with this refactoring.

The only check that I should add here is an assert on UpperBound being positive and not exceeding INT_MAX.
Other than that I dont see what makes int64_t unclear here.

adding assert

This revision was automatically updated to reflect the committed changes.