[PATCH] D16890: Fix PR25339: ARM Constant Island

[Zhao, Weiming via llvm-commits]

Hi Renato,

I'll post the test case shortly. Since it's based on proprietary code, 
it needs to complete some internal review process.
So far, I can only give a brief description of the problematic code. 
Please see https://llvm.org/bugs/show_bug.cgi?id=25339

The number 15 is half of the max threshold of 30.
I did a measurement of building SPEC and the highest iteration number I 
see is 3.

Thanks,
Weiming

On 2/5/2016 5:00 AM, Renato Golin wrote:
> rengolin added a comment.
>
> Hi Weiming,
>
> I'd like to know how you got to the magic number of 15. So, showing a small program that doesn't converge, then showing that this change makes it at least stop and produce whatever result is acceptable.
>
> Constant islands are a major pain to get right because some relocations are only handled later and merging two islands can grow the distance of a third beyond the range of some load far away. In the same way, *not* merging could cause the same effects.
>
> Would be good to see the results of compiling large pieces of code (say, LNT, Chromium, Firefox) which have plenty of opportunities to get this wrong. Without tests, it's hard to see what kind of problem you're trying to avoid, here.
>
> cheers,
> --renato
>
>
> ================
> Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:459
> @@ -458,3 +458,3 @@
>       for (unsigned i = 0, e = CPUsers.size(); i != e; ++i)
> -      CPChange |= handleConstantPoolUser(i);
> +      CPChange |= handleConstantPoolUser(i, NoCPIters >= 15 /*Ignore Growth*/);
>       if (CPChange && ++NoCPIters > 30)
> ----------------
> Shouldn't this 15 be in a constant somewhere? Maybe a hidden command line options, which defaults to 15, but can be changed?
>
> ================
> Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:1313
> @@ -1311,1 +1312,3 @@
>       // we don't insert more branches than necessary.
> +    // When IgnoreGrowth is true, we try to find the lowerest address after (or
> +    // equal to) user MI's BB no matter of padding growth.
> ----------------
> s/lowerest/lowest/
>
> ================
> Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:1327
> @@ -1323,1 +1326,3 @@
> +        return true;
>         // Keep looking unless it is perfect.
> +      if (!IgnoreGrowth && BestGrowth == 0)
> ----------------
> Update the comment to reflect the new flag
>
>
> Repository:
>    rL LLVM
>
> http://reviews.llvm.org/D16890
>
>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation