[PATCH] D16890: Fix PR25339: ARM Constant Island

[Renato Golin via llvm-commits]

rengolin added a comment.

In http://reviews.llvm.org/D16890#345099, @weimingz wrote:

> I'll post the test case shortly. Since it's based on proprietary code, 
>  it needs to complete some internal review process.


I completely understand. No worries.

> So far, I can only give a brief description of the problematic code. 

>  Please see https://llvm.org/bugs/show_bug.cgi?id=25339


Ah, excellent! That bug was on my radar, and I'm glad someone is looking at it. When you submit a review based on a bug, it's good to put the bug number like "Addresses bug #NNNN", so we can grab the rest of the context.

> The number 15 is half of the max threshold of 30.


Right. If the max is hard-coded, then maybe say max/2 in there?

It's also not clear to me why 15 is better than 30. If the algorithm fails to converge on both, than clearly either hard-coded numbers would be ok as long as the safe option is implemented. This patch seems to be doing both: forcing a convergence, even if a bad one (which would also work with 30 iterations, I think), and reducing the threshold to 15 (which doesn't make much sense per se).

> I did a measurement of building SPEC and the highest iteration number I 

>  see is 3.


This means that the application itself is problematic in that respect, so any number beyond 5 would be a good bet, with higher numbers better because they would only hit the worse cases.

What's the impact if you keep the original threshold?

cheers,
--renato


Repository:
  rL LLVM

http://reviews.llvm.org/D16890