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

[Stepan Dyatkovskiy]

I fixed code heuristics. How it works now:

1. Calculate average number of produced instructions and basics blocks:
number-of-instructions=curret-loop->number-of-instructions * 
unswitched-number
number-of-bb=curret-loop->number-of-bb * unswitched-number

2. If number-of-instructions > Threshold || number-of-bb*5 > Threshold, 
stop unswitching.

By default Threshold is 50. But user can set custom threshold using 
-loop-unswitch-threshold <custom-threshold> option. This option existed 
before my patch, and I kept it without changes though.

I compiled ffmpeg (ffmpeg.org) with llvm + clang toolchain (with and 
without my patch). I got the next results:

Without patch:   ffmpeg size is 7369612 bytes.
Threshold = 50:  ffmpeg size is 7349132 bytes (less then in ToT version).
Threshold = 200: ffmpeg size is 7439244 bytes.

I also checked the ffmpeg build time in all cases it was 2 mins, 35 secs.
Transcoding time with ffmpeg (mp3 -> mp2) in all cases was also the 
same: process took 1 min, 19 secs.

I added unit test that checks unswitch kicking in case of insufficient 
size. Unit tests and fixed patch (default threshold is 50) are attached 
to this post.

Thanks.
-Stepan.

Dan Gohman wrote:
>
> On Dec 7, 2011, at 5:54 AM, Stepan Dyatkovskiy wrote:
>
>> Dan Gohman wrote:
>>
>>> 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.
>>
>> The main reason to suspect a bug is that the pass doesn't work as it was planned initially. I made this conclusion after reading the code and its comments. Especially this ones (LoopUnswitch.cpp, string #277):
>>         // FIXME: this should choose the most expensive case!
>>         // FIXME: scan for a case with a non-critical edge?
>> Programmer placed the stub by now:
>> 	Constant *UnswitchVal = SI->getCaseValue(1);
>>
>> Probably he missed one thing. Since we selected 1-st case always, all other cases will never unswitched. But there is no comments about it. This "feature" looks unplanned. If not - ok, it is not a bug then. I proposed to insert additional comments though.
>>
>> Size heuristics currently analyses the size of loop to be unswitched. If number of instructions, or number of BBs exceeds some threshold, then the loop will skipped. We can also control the number cases to be unswitched.
>
> Oh, I don't doubt it may a bug. And there's no master plan behind
> non-trivial loop unswitching for switches. My point was just that
> it's been the way it is for a long time, and possibly other things
> have become accustomed to it, or accidentally reliant on it,
> working that way.
>
>>
>>> 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.
>>
>> Before unswitchment, loop executes switch N times. The switch is lowered to several "icmp" checks (let it be "n"). Depending on lowering algorithm selected the "n" is either linear or logarithmic function on cases number ( n=k*num_cases or n=k*log(num_cases) ). Summary we have N*n "icmp" checks.
>>
>> After unswitchment all checks will moved out of loop. And we got 1*n_us "icmp" checks in this case. But the "n_us" is always linear after unswitchment. So if (1*n_us<  N*n) then performance will increased. Else - it will decreased.
>
> On most architectures, those checks will usually be predictable -- the
> conditions are loop-invariant after all -- and thus fairly cheap, depending
> on what else is in the loop. But this isn't important here.
>
>>
>>> If you can confirm
>>> that that's working as expected, it should be fine.
>>
>> Did you mean the regression tests? If yes - they are attached in initial post. These tests checks that all working as expected for loop with single switch instruction and for loop with two switches.
>>
>> In my patch I fixed the stub described above. I also can improve the size heuristics if you need.
>
>
> I just meant that it would be good to have testcases in which the code
> size heuristic actually has to kick in and prevent it from unswitching
> out of control.
>
> Also, if you could do a rough check of code size on some general real-world
> application code, that would be good too.
>
> With these precautions, I think the patch is fine.
>
> Dan
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: loop-unswitch-unswitchvals-regtests.patch
Type: text/x-patch
Size: 13038 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111210/f225ecb7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: loop-unswitch-unswitchvals.patch
Type: text/x-patch
Size: 7121 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111210/f225ecb7/attachment-0001.bin>