[llvm-dev] [GlobalISel][AArch64] Toward flipping the switch for O0: Please give it a try!

Mon Nov 13 10:26:43 PST 2017

Hi Kristof,

> On Nov 13, 2017, at 9:10 AM, Kristof Beyls <Kristof.Beyls at arm.com> wrote:
> 
> Hi Quentin,
> 
> My only remaining concern is around ABI compatibility.
> The following commit seems to indicate that in the previous round of evaluation, we didn’t find an existing ABI compatibility issue:
> http://llvm.org/viewvc/llvm-project?view=revision&revision=311388 <http://llvm.org/viewvc/llvm-project?view=revision&revision=311388>.
> I haven’t looked into the details of this issue - so maybe I’m worried over nothing?

No, you’re right. The problem with ABI is if you are consistently wrong, then you won’t notice :).

> 
> I’m wondering if since then on your side you did any testing around ABI compatibility?
> E.g. building software where you semi-randomly build some functions through GlobalISel and some functions through DAGISel?

Justin will look into that. Clang has utility script for that utils/ABITest.

Given we will only be able to check iOS ABI, you may want to follow the same kind of validation on your side.

I let you sync up with Justin for the method.

Cheers,
-Quentin

> 
> Thanks,
> 
> Kristof
> 
>> On 8 Nov 2017, at 00:42, Quentin Colombet via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>> Hi all,
>> 
>> I’d like to resurrect this thread and ask if people are on board for enabling this by default for AArch64 O0.
>> 
>> 
>> *** What Changed Since June? ***
>> 
>> - We added a way to describe the legalization actions for non-power-of-2 
>> - We gave a tutorial that covers the best practices to target GlobalISel
>> - We improved the TableGen backend to reuse existing SDISel patterns
>> - We built and ran huge internal software with GISel
>> - We evaluated the performance of GISel and are confident things are in a good shape (with https://reviews.llvm.org/D39034 <https://reviews.llvm.org/D39034>) and moving forward would look even better (see the last LLVM Dev talk: GlobalISel: Present, Past, and Future when it is available)
>> 
>> 
>> *** So What’s he Plan? ***
>> 
>> - Switch the default instruction selector to GISel for AArch64 at O0
>> - Enable the fallback path by default for AArch64 (with warnings enabled when that path is hit)
>> - Provide a clang option to turn GISel off
>> 
>> What do you think?
>> 
>> Thanks,
>> -Quentin
>> 
>>> On Jun 16, 2017, at 4:43 PM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>>> 
>>> Hi all,
>>> 
>>> We had some internal discussions about flipping the default for O0 and we concluded that we wanted to postpone it.
>>> 
>>> 
>>> *** Why Is That? ***
>>> 
>>> We don’t want to send the wrong message that GlobalISel’s design is set in stone and ready for broader adoption.
>>> In particular,
>>> 1. The APIs are still evolving and can still possibly change significantly
>>> 2. The TableGen backend to reuse the existing SD patterns is still at its early stage
>>> 3. We want to investigate closely the performance of global-isel (compile-time, runtime, code size, fallbacks)
>>> 
>>> The rationale behind those items is that we want to minimize the pain of moving forward for everybody. We also want the out-of-the-box experience to be pleasant (like all/most of the tablegen patterns just work, we have documentation on how to target a new backend, etc.) Finally, we want to gain confidence we are going to be able to address the performance issues we have with the current design and if not, derive a plan for that.
>>> 
>>> We purposely left out of the conversation what will be the right time and requirements to flip the switch. We want to gather more data first. Your help would be appreciated!
>>> 
>>> 
>>> *** Short-Term Proposal ***
>>> 
>>> What we would like to do instead short-term is:
>>> A. Repurpose or create an option “-aarch64-enable-global-isel-at-O” to enable GISel with fallbacks and warnings enables (i.e., equivalent of -global-isel -global-isel-abort=2)
>>> B. Advertise this option in the next open source release to allow compiler enthusiastic to try it and report problems
>>> C. Have GISel always built so we can push thing in the right place, MachineVerifier in mind, and stop doing some weird gymnastic
>>> 
>>> What do people think?
>>> 
>>> 
>>> *** Your Help Is Needed ***
>>> 
>>> - Please share your experience in using the GISel APIs and how we can make them better. Moving forward we’ll have those conversations on open source instead of internally/with a narrower audience.
>>> - Report any performance problem you identify
>>> - Propose patches!
>>> 
>>> Cheers,
>>> -Quentin
>>> 
>>> 
>>> 
>>>> On Jun 16, 2017, at 3:06 PM, Quentin Colombet via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>>>> 
>>>>> 
>>>>> On Jun 14, 2017, at 7:27 AM, Diana Picus <diana.picus at linaro.org <mailto:diana.picus at linaro.org>> wrote:
>>>>> 
>>>>> On 12 June 2017 at 18:54, Diana Picus <diana.picus at linaro.org <mailto:diana.picus at linaro.org>> wrote:
>>>>> Hi all,
>>>>> 
>>>>> I added a buildbot [1] running the test-suite with -O0 -global-isel. It runs into the same 2 timeouts that I reported previously on this thread (paq8p and scimark2). It would be nice to make it green before flipping the switch.
>>>>> 
>>>>> 
>>>>> I did some more investigations on a machine similar to the one running the buildbot. For paq8p and scimark2, I get these results for O0:
>>>>> 
>>>>> PAQ8p:
>>>>> Fast isel: 666.344
>>>>> Global isel: 731.384
>>>>> 
>>>>> SciMark2-C:
>>>>> Fast isel: 463.908
>>>>> Global isel: 496.22
>>>>> 
>>>>> The current timeout is 500s (so in this particular case we didn't hit it for scimark2, and it ran successfully to completion). I don't think the difference between FastISel and GlobalISel is too atrocious, so I would propose increasing the timeout for these 2 benchmarks. I'm not sure if we can do this on a per-bot basis, but I see some precedent for setting custom timeout thresholds for various benchmarks on different architectures (sometimes with comments that it's done so we can run O0 on that particular benchmark). 
>>>>> 
>>>>> Something along these lines works:
>>>>> https://reviews.llvm.org/differential/diff/102547/ <https://reviews.llvm.org/differential/diff/102547/>
>>>>> 
>>>>> What do you guys think about this approach?
>>>> 
>>>> Looks reasonable to me.
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Diana
>>>>> 
>>>>> PS: The buildbot is using the Makefiles because that's what our other AArch64 test-suite bots use. Moving all of them to CMake is a transition for another time.
>> 
>> _______________________________________________
>> 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/20171113/d2e4bfad/attachment.html>