[llvm-commits] [LLVM, loop-unswitch, bugfix for #11429] Wrong behaviour for switches.

[Dan Gohman]

On Dec 5, 2011, at 5:03 AM, Stepan Dyatkovskiy wrote:

> Hi Dan. This bug is described in details (with examples) here:
> http://llvm.org/bugs/show_bug.cgi?id=11429
> 
> Regarding to your questions..
> 
>> It's even possible that this bug is
>> accidentally helping the code by preventing it from unswitching too
>> much in the presence of switches.
>> Do you have an idea on what impact
>> this patch has on code size, and performance, in general?
> 
> Happily this bug helps to keep the code size small. But LoopUnswitch::UnswitchIfProfitable method already controls the produced code size. Please, see LoopUnswitch.cpp, string #446 for more details. I think that if we need to improve the "restrictioning" of produced code size we need to implement this improvement instead of keeping some strange code.

This sounds good. How sure are you that the existing size heuristics
are actually kicking in for the new unrollings? I don't have a reason
to suspect a bug, other than that you're asking code to work in cases
that it hasn't before.

> 
> About impact on code size.
> For switch with N cases (+ 1 default) we got N new loops. If you wish I can present the .ll code that should be produced after optimization.
> 
> Impact on performance.
> The main purpose of this optimization is to move out of loop the switches. Each unswitched case increases the performance.

Each unswitched case has the potential to increase performance,
if conditions are favorable. But if code size is increased, there's
also the possibility of decreased performance. But I think you're
right, the existing throttle ought to handle that. If you can confirm
that that's working as expected, it should be fine.

> 
> About releaseMemory and CloneUnswitchedVals.
> This methods the part of unswitch info cloning for new loops. It is also described in bug #11429.
> 
> > CloneUnswitchedVals doesn't actually need to iterate over the
> > instructions in the block to find the SwitchInst. If there's a
> > SwitchInst present, it'll be the Terminator instruction.
> OK. You're right. Please find the fixed patch.

Thanks,

Dan