[llvm-dev] RFC: Building GlobalISel by default

Tue Jan 24 17:10:44 PST 2017

On Thu, Jan 19, 2017 at 1:10 PM Jim Grosbach <grosbach at apple.com> wrote:

> On Jan 19, 2017, at 11:59 AM, Quentin Colombet <qcolombet at apple.com>
> wrote:
>
> Hi all,
>
> Trying to summarize the concerns along side my answers but I believe we
> are good to do the switch to build GlobalISel by default. In particular,
> Renato and I exchanged offline and he tried doing the switch on some of the
> bots he maintained and did not see any overhead/problem for that.
> Let me know if you disagree.
>
>
> * What is the impact on compile time?
>
> Negligible: ~5s for a 19min build. (See
> http://lists.llvm.org/pipermail/llvm-dev/2017-January/109185.html for the
> details.)
>
>
> * What is the impact on binary size?
>
> Negligible: 0 to 1M on a 37M to 104M. (See
> http://lists.llvm.org/pipermail/llvm-dev/2017-January/109185.html for the
> details.)
>
>
> * How likely GlobalISel will break on non-related change?
>
> Unlikely: GlobalISel's APIs are fairly isolated or work on pretty well
> established low-level APIs. (See
> http://lists.llvm.org/pipermail/llvm-dev/2017-January/109154.html for
> details.)
>
>
> * Why GlobalISel wasn’t built from the start?
>
> One of the requirement was that we don’t impact negatively the footprint
> of the compiler. Given we were not sure where we were going and in
> particular we temporarily increased the size of the MachineInstrs and some
> other core classes during the prototype, we decided to turn it off by
> default. Those problems have been fixed now.
>
>
> * How is the existing GlobalISel implement viewed?
>
> Last year we finished the proof-of-concept and are moving toward
> production quality now. We are doing reviews like any other part of the
> code and rewrite what we believe needs to be rewritten. I’ll go into more
> details regarding the status in another thread, but one of the thing we are
> pushing right now is tablegen support in GlobalISel so that we can just
> reuse the existing td form SDISel.
>
>
> To reinforce Quentin’s point here, if folks have concerns about the
> suitability of areas of the code for long term maintainability, elegance of
> design or implementation, or anything else regarding the code quality,
> please say so. Deep and thorough review is essential to long term success.
>
>
Sadly I didn't have time to review a prototype and am a little lacking in
review time right now. I'll have to hope for the best :)

-eric

>
>
> Let me know if I didn’t address some of the concerns.
>
> Cheers,
> -Quentin
>
> On Jan 18, 2017, at 10:56 AM, Eric Christopher <echristo at gmail.com> wrote:
>
>
>
> On Wed, Jan 18, 2017 at 9:13 AM David Blaikie via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> These concerns sound applicable to the situation when GlobalISel is turned
> on by default and has an effect on code generation.
>
> While it's a library with tests like any other LLVM component I don't see
> these concerns as being significantly greater risk than any other library
> in LLVM.
>
> I'm honestly surprised GlobalISel was developed in such a way that it
> hasn't been built and tested by default for all developers from Day 1. The
> new pass manager and the ORC JIT are examples of this - there was never any
> question about when to enable building and testing them. They were built
> and tested from the moment they went in tree.
>
>
> A lot of this is because GlobalISel was a "prototype" that would just be
> easier to work on in top of tree rather than something that was made with
> the idea of going live originally. Arguably code review requirements were a
> bit more lax, etc. This appears to have changed recently, but I'm not sure
> what changed in how the existing GlobalISel implementation is viewed.
>
> So, what's up? :)
>
> -eric
>
>
>
> The question of when to turn these things (GlobalISel, New PM, ORC, etc)
> is a very different question and one I'd expect to be staged in the way
> you're describing, Renato. Provide a flag, off by default. Ask/encourage
> people to test it out. Eventually flip the default - encouraging people to
> unflip manually and report if they have trouble. Maintain this state until
> reported issues are addressed/wait for a grace period, then remove the
> flag/old code entirely.
>
>
> On Sun, Jan 15, 2017 at 5:15 AM Renato Golin via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> On 15 January 2017 at 04:24, Michael Kuperstein
> <michael.kuperstein at gmail.com> wrote:
> > Regarding "we shouldn't enable it because it will make the bots slower" -
> > well, yes, but that's just postponing the inevitable. We will enable
> > GlobalISel eventually, and there will probably be a very long time-frame
> > during which both are enabled concurrently.
>
> No magic solutions expected. I just don't want to crank all bots up to
> 11 on day one. We start with the fast bots, when they're happy, we
> move on until we're confident that the whole thing is stable, then we
> make it build by default.
>
> There are two problems here:
>
> 1. Environmental issues.
>
> We do find more of those than we'd hope. Different compilers, linkers,
> libraries that produce not only crashes, but unstable builds and
> unpredictable or irreproducible test failures. The environment on our
> bots are wildly different (on purpose) and dealing with different
> unpredictable issues all at once is something that I'm not willing to
> take on lightly. Due to the nature of buildbots and some of our
> hardware and the fact that virtually all ARM/AArch64 bots are ours,
> this will be something that Linaro will pay the price in full.
>
> 2. Experimental nature.
>
> If the past serves as a guide, experimental features at the core of
> LLVM will be largely independent until they start to merge, when they
> can become a huge issue and be in merging state for months, if not
> years, or worse, never be merged. Right now, we're not at that state,
> we're still largely independent, so your worries regarding the
> development tests (which also hit buildbots at a different pace
> because of the environment issues above), will be lower for now, but
> will increase massively soon enough.
>
> There is the argument that having the bots building means we'll have
> *less* teething issues and the people building GlobalISel (which also
> includes Linaro) will have a lot less work. But there's a cost to be
> paid, and that cost is a lot higher on the ARM/AArch64 side than x86,
> just because most developers use an x86 as their main machine.
>
>
> > Still, as a strawman proposal - would it make sense to add
> > LLVM_BUILD_GLOBAL_ISEL_EXPERIMENTAL, or something of that sort? The more
> > mature ports will build under LLVM_BUILD_GLOBAL_ISEL (which will be
> enabled
> > by default), the less mature ones only under
> > LLVM_BUILD_GLOBAL_ISEL_EXPERIMENTAL (which won't be), and moving a port
> from
> > GLOBAL_ISEL_EXPERIMENTAL to GLOBAL_ISEL would be equivalent to marking a
> > regular backend non-experimental. We can start with just AArch64 in the
> > non-experimental category, and move the others as they become more
> stable.
> > Or is this too much complexity for what we gain?
>
> It is.
>
> We already separate that by not building any other back-end than ARM
> and AArch64. So I think we can do that without the added complexity.
>
> I'm not trying to block the move, I'm just trying to make it smother
> and raise awareness that the cost we pay for any large move is not
> well distributed.
>
> For the past few months I have reverted patches that only broke the
> ARM bots and it was clear on most of those instances which patch was
> to blame, but people didn't bother and after 8, 12 sometimes 24 hours,
> I reverted. This is not a nice place to be in, and given that
> resurgence lately, I am honestly afraid adding more things will make
> it worse.
>
> A red bot can't catch new errors. A slow red bot can go on red for
> weeks (and we have seen it numerous times) because once you fix bug
> #1, #2 has made #3 appear and not be warned.
>
> This is not everyone, and I do appreciate all the people that helped
> us and worried about the breakages, but it's a trend that is
> increasing, not decreasing. Our team is too small to cope with further
> increases, so if the community is willing to do its part, I think we
> can make it work.
>
> But it has to be slow and steady.
>
> cheers,
> --renato
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170125/1ed7e489/attachment-0001.html>