llvm dev - Jan 2017 - RFC: Building GlobalISel by default

Hey Quentin, reads like you got the community nod and can start the engine …

Cheers
Gerolf
> On Jan 24, 2017, at 5:10 PM, Eric Christopher via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 
> 
> On Thu, Jan 19, 2017 at 1:10 PM Jim Grosbach <grosbach at apple.com
<mailto:grosbach at apple.com>> wrote:
>> On Jan 19, 2017, at 11:59 AM, Quentin Colombet <qcolombet at
apple.com <mailto: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
<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
<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
<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 <mailto: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 <mailto: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 <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>> On 15 January 2017 at 04:24, Michael Kuperstein
>>> <michael.kuperstein at gmail.com <mailto: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 <mailto:llvm-dev at
lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<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/6feee14e/attachment.html>

Thanks all the feedbacks.

I moved forward, the switch happened in r293232.

For incremental buildbots, I believe we are going to need to kick a clean build
for them to pick it up.
@Galina, is it something you can do?

Cheers,
-Quentin
> On Jan 25, 2017, at 10:50 AM, Gerolf Hoflehner <ghoflehner at
apple.com> wrote:
> 
> Hey Quentin, reads like you got the community nod and can start the engine
…
> 
> Cheers
> Gerolf
> 
>> On Jan 24, 2017, at 5:10 PM, Eric Christopher via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>> 
>> 
>> On Thu, Jan 19, 2017 at 1:10 PM Jim Grosbach <grosbach at apple.com
<mailto:grosbach at apple.com>> wrote:
>>> On Jan 19, 2017, at 11:59 AM, Quentin Colombet <qcolombet at
apple.com <mailto: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
<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
<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
<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 <mailto: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 <mailto: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 <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>>> On 15 January 2017 at 04:24, Michael Kuperstein
>>>> <michael.kuperstein at gmail.com
<mailto: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 <mailto:llvm-dev at
lists.llvm.org>
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
>>> 
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org <mailto: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/20170126/7f3b30c7/attachment.html>

> On Jan 26, 2017, at 4:12 PM, Quentin Colombet <qcolombet at
apple.com> wrote:
> 
> Thanks all the feedbacks.
> 
> I moved forward, the switch happened in r293232.
Great!
> 
> For incremental buildbots, I believe we are going to need to kick a clean
build for them to pick it up.
> @Galina, is it something you can do?
> 
> Cheers,
> -Quentin
> 
>> On Jan 25, 2017, at 10:50 AM, Gerolf Hoflehner <ghoflehner at
apple.com <mailto:ghoflehner at apple.com>> wrote:
>> 
>> Hey Quentin, reads like you got the community nod and can start the
engine …
>> 
>> Cheers
>> Gerolf
>> 
>>> On Jan 24, 2017, at 5:10 PM, Eric Christopher via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>> 
>>> 
>>> 
>>> On Thu, Jan 19, 2017 at 1:10 PM Jim Grosbach <grosbach at
apple.com <mailto:grosbach at apple.com>> wrote:
>>>> On Jan 19, 2017, at 11:59 AM, Quentin Colombet <qcolombet at
apple.com <mailto: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
<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
<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
<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 <mailto: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 <mailto: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 <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>>>> On 15 January 2017 at 04:24, Michael Kuperstein
>>>>> <michael.kuperstein at gmail.com
<mailto: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 <mailto:llvm-dev at
lists.llvm.org>
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
>>>>> _______________________________________________
>>>>> LLVM Developers mailing list
>>>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
>>>> 
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<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/20170126/f7020f35/attachment.html>

On 27 January 2017 at 00:12, Quentin Colombet <qcolombet at apple.com>
wrote:
> Thanks all the feedbacks.
>
> I moved forward, the switch happened in r293232.
Thanks!

> For incremental buildbots, I believe we are going to need to kick a clean
> build for them to pick it up.
> @Galina, is it something you can do?
Unfortunately, no. The logic to re-create the ninja files is in the
builder itself, not the master.

I don't know how many people apart from us are using dirty builds
(they're unstable by definition), but I'll be recycling them
throughout the weekend.

Will let you know when I'm done.

cheers,
--renato