[PATCH] D31085: [InlineCost] Increase the cost of Switch

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/Analysis/InlineCost.cpp:1057
+    uint64_t SwitchCost = ExpectedNumberOfCompare * 2 * InlineConstants::InstrCost;
+    Cost += std::min((uint64_t)INT_MAX, SwitchCost + (uint64_t) Cost);
+    return false;
----------------
hans wrote:
> junbuml wrote:
> > hans wrote:
> > > hans wrote:
> > > > Aren't you adding `Cost` twice here? You're doing `Cost +=` and also `SwitchCost + Cost`.
> > > > 
> > > > Oh, Cost is just a regular int; yeah then I see how it can overflow. But std::min is returnning an uint64_t here, so it seems you're still not handling the overflow?
> > > Oh never mind, you're capping the uint64_t at INT_MAX so it shuold work. Probably still don't want to do `Cost +=` though.
> > Thanks for this. Yes, it should be Cost =, instead of Cost +=.
> Cool, makes sense now. Are the `(uint64_t)` casts strictly necessary though? SwitchCost is already `uint64_t` so I'd imagine `INT_MAX` to get promoted and everything to work out?
We need (uint64_t)INT_MAX, but we don't need (uint64_t) Cost as  SwitchCost is uint64_t.


https://reviews.llvm.org/D31085