llvm dev - May 2017 - The state of ARMConstantIslandPass in 4.0.[01]

Hi,

We at Rust are using LLVM for our codegen. Since Rust version 1.19.0,
which should ship in July 21st, we are using a slightly patched
version of LLVM 4.0.

The transition to LLVM 4.0 had been fairly pain-free on x86 (at least
excluding the standard suite of assertion failures using MSVC SEH, but
that's fairly under control), but we have encountered several scary
bugs in ARM, most specifically in ARMConstantIslandPass.cpp:

 - https://github.com/rust-lang/rust/issues/41672
 - https://github.com/rust-lang/rust/issues/42248

The former of these is a fairly benign assertion failure, but the
latter is a scary "in the wild" wrong codegen bug, which can really
mess up with programmers.

Both of the bugs have fixes in trunk:
r295964 - Fix assertion failure in ARMConstantIslandPass.
r297871 - ARM: avoid clobbering register in v6 jump-table expansion.

I can easily backport the fixes for these 2 bugs into our LLVM branch.
However, while looking at the diff between 4.0 and trunk I've seen
several more fixes to "scary-looking" bugs:

r294949 - [Thumb-1] TBB generation: spot redefinitions of index register
r295816 - [ARM] Fix constant islands pass.
r299634 - [ARM] Remove a dead ADD during the creation of TBBs
    (that one looks more like an optimization than a wrong-codegen
thing, but including for completeness)
r300870 - [Thumb-1] Fix corner cases for compressed jump tables

After seeing the previous ones, I'm worried these other fixes might
also fix some "in the wild" wrong codegen bugs. However, I'm not
comfortable backporting so many patches by myself. We are open to
moving to being based on LLVM 4.0.1 when it is released, but it does
not seem to include *any* of the above fixes.

Are these fixes important? Should we backport them? Do we need to keep
anything in mind?

Thanks in advance,
    - Ariel

On 05/29/2017 05:41 PM, Ariel Ben-Yehuda via llvm-dev
wrote:
> Hi,
> 
> We at Rust are using LLVM for our codegen. Since Rust version 1.19.0,
> which should ship in July 21st, we are using a slightly patched
> version of LLVM 4.0.
> 
> The transition to LLVM 4.0 had been fairly pain-free on x86 (at least
> excluding the standard suite of assertion failures using MSVC SEH, but
> that's fairly under control), but we have encountered several scary
> bugs in ARM, most specifically in ARMConstantIslandPass.cpp:
> 
>  - https://github.com/rust-lang/rust/issues/41672
>  - https://github.com/rust-lang/rust/issues/42248
> 
> The former of these is a fairly benign assertion failure, but the
> latter is a scary "in the wild" wrong codegen bug, which can
really
> mess up with programmers.
> 
> Both of the bugs have fixes in trunk:
> r295964 - Fix assertion failure in ARMConstantIslandPass.
> r297871 - ARM: avoid clobbering register in v6 jump-table expansion.
> 
> I can easily backport the fixes for these 2 bugs into our LLVM branch.
> However, while looking at the diff between 4.0 and trunk I've seen
> several more fixes to "scary-looking" bugs:
> 
> r294949 - [Thumb-1] TBB generation: spot redefinitions of index register
> r295816 - [ARM] Fix constant islands pass.
> r299634 - [ARM] Remove a dead ADD during the creation of TBBs
>     (that one looks more like an optimization than a wrong-codegen
> thing, but including for completeness)
> r300870 - [Thumb-1] Fix corner cases for compressed jump tables
> 
> After seeing the previous ones, I'm worried these other fixes might
> also fix some "in the wild" wrong codegen bugs. However, I'm
not
> comfortable backporting so many patches by myself. We are open to
> moving to being based on LLVM 4.0.1 when it is released, but it does
> not seem to include *any* of the above fixes.
> 
> Are these fixes important? Should we backport them? Do we need to keep
> anything in mind?
> 
It's probably too late for these to make it into the 4.0.1 release,
since the merge deadline is today, but please submit merge requests
for these anyway (preferred way is to use the utils/release/merge-request.sh
script), because it will help us determine if we need a 4.0.2 release.

Thanks,
Tom
> Thanks in advance,
>     - Ariel
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

Opened merge requests:
https://bugs.llvm.org/show_bug.cgi?id=33213
https://bugs.llvm.org/show_bug.cgi?id=33214
https://bugs.llvm.org/show_bug.cgi?id=33215
https://bugs.llvm.org/show_bug.cgi?id=33216
https://bugs.llvm.org/show_bug.cgi?id=33217
https://bugs.llvm.org/show_bug.cgi?id=33218

On Tue, May 30, 2017 at 1:27 AM, Tom Stellard <tstellar at redhat.com>
wrote:
> On 05/29/2017 05:41 PM, Ariel Ben-Yehuda via llvm-dev wrote:
>> Hi,
>>
>> We at Rust are using LLVM for our codegen. Since Rust version 1.19.0,
>> which should ship in July 21st, we are using a slightly patched
>> version of LLVM 4.0.
>>
>> The transition to LLVM 4.0 had been fairly pain-free on x86 (at least
>> excluding the standard suite of assertion failures using MSVC SEH, but
>> that's fairly under control), but we have encountered several scary
>> bugs in ARM, most specifically in ARMConstantIslandPass.cpp:
>>
>>  - https://github.com/rust-lang/rust/issues/41672
>>  - https://github.com/rust-lang/rust/issues/42248
>>
>> The former of these is a fairly benign assertion failure, but the
>> latter is a scary "in the wild" wrong codegen bug, which can
really
>> mess up with programmers.
>>
>> Both of the bugs have fixes in trunk:
>> r295964 - Fix assertion failure in ARMConstantIslandPass.
>> r297871 - ARM: avoid clobbering register in v6 jump-table expansion.
>>
>> I can easily backport the fixes for these 2 bugs into our LLVM branch.
>> However, while looking at the diff between 4.0 and trunk I've seen
>> several more fixes to "scary-looking" bugs:
>>
>> r294949 - [Thumb-1] TBB generation: spot redefinitions of index
register
>> r295816 - [ARM] Fix constant islands pass.
>> r299634 - [ARM] Remove a dead ADD during the creation of TBBs
>>     (that one looks more like an optimization than a wrong-codegen
>> thing, but including for completeness)
>> r300870 - [Thumb-1] Fix corner cases for compressed jump tables
>>
>> After seeing the previous ones, I'm worried these other fixes might
>> also fix some "in the wild" wrong codegen bugs. However,
I'm not
>> comfortable backporting so many patches by myself. We are open to
>> moving to being based on LLVM 4.0.1 when it is released, but it does
>> not seem to include *any* of the above fixes.
>>
>> Are these fixes important? Should we backport them? Do we need to keep
>> anything in mind?
>>
>
> It's probably too late for these to make it into the 4.0.1 release,
> since the merge deadline is today, but please submit merge requests
> for these anyway (preferred way is to use the
utils/release/merge-request.sh
> script), because it will help us determine if we need a 4.0.2 release.
>
> Thanks,
> Tom
>
>> Thanks in advance,
>>     - Ariel
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>

On 29 May 2017 at 22:41, Ariel Ben-Yehuda via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> Are these fixes important? Should we backport them? Do we need to keep
> anything in mind?
Hi Ariel,

Those fixes are important, but as Tom said, only for 4.0.2 if we do one.

However, they'll need to be looked at with care, as they fix some odd
bugs in the compiler and I'm not sure how stable they really are.

They may behave well in trunk because of other patches that were
noticed and fixed, but back-porting just those patches may introduce
instabilities that weren't fixed in 4.0.

Given that we release a new version every 6 months, and that we
(Still) don't have (community) support for older versions, you may be
better off using 5.0 instead.

Thanks for creating the merge requests, though. Let's look at them
when/if 4.0.2 comes along.

cheers,
--renato

On 5/30/17, Renato Golin <renato.golin at linaro.org>
wrote:
> On 29 May 2017 at 22:41, Ariel Ben-Yehuda via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>> Are these fixes important? Should we backport them? Do we need to keep
>> anything in mind?
>
> Hi Ariel,
>
> Those fixes are important, but as Tom said, only for 4.0.2 if we do one.
>
> However, they'll need to be looked at with care, as they fix some odd
> bugs in the compiler and I'm not sure how stable they really are.
>
> They may behave well in trunk because of other patches that were
> noticed and fixed, but back-porting just those patches may introduce
> instabilities that weren't fixed in 4.0.
>
> Given that we release a new version every 6 months, and that we
> (Still) don't have (community) support for older versions, you may be
> better off using 5.0 instead.
>
> Thanks for creating the merge requests, though. Let's look at them
> when/if 4.0.2 comes along.
>
> cheers,
> --renato
>
> They may behave well in trunk because of other patches that were
noticed and fixed, but back-porting just those patches may introduce
instabilities that weren't fixed in 4.0.

That's what's worrying me. Because we don't want to ship a version
of
Rust on which developing on ARM is a minefield, we have to apply the
first 2 patches. However, it looks that we might also need to apply
the other patches, and I'm worried about whether and how can they be
safely backported. Could the patch authors pitch in?
> Given that we release a new version every 6 months, and that we (Still)
don't have (community) support for older versions, you may be better off
using 5.0 instead.
In our experience, every new version of LLVM both requires some
porting work and introduces new issues in non-P1 targets that need to
be fixed. We'll upgrade to LLVM 5.0 as soon as it gets released and we
port and apply the fixes. In the meanwhile we'll like a version in
which ARM is less of a minefield.

 - Ariel