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

Tue Nov 14 09:29:11 PST 2017

Thanks Oliver.
I’ll have a look. This typically means that we miss a mapping for this type/instruction, which is not surprising given how little code we have we fp16.

> On Nov 14, 2017, at 2:27 AM, Oliver Stannard <oliver.stannard at arm.com> wrote:
> 
> Hi Quentin,
>  
> I’ve started running an ABI test suite with global isel on AArch64, and while it hasn’t found any ABI issues it has hit an assertion in clang when using the __fp16 type. Here’s a reproducer:
>  
>   __fp16 pass_f16(__fp16 p) {
>     return p;
>   }
>  
>   $ /work/llvm/build/bin/clang --target=aarch64-arm-none-eabi -march=armv8-a -c test.c -O0 -mllvm -global-isel -mllvm -global-isel-abort=0
>   clang-6.0: /work/llvm/llvm/lib/CodeGen/GlobalISel/RegisterBankInfo.cpp:446: static void llvm::RegisterBankInfo::applyDefaultMapping(const llvm::RegisterBankInfo::OperandsMapper &): Assertion `OrigTy.getSizeInBits() == NewTy.getSizeInBits() && "Types with difference size cannot be handled by the default " "mapping"' failed.
>   #0 0x000000000362a764 PrintStackTraceSignalHandler(void*) (/work/llvm/build/bin/clang-6.0+0x362a764)
>   #1 0x000000000362aac6 SignalHandler(int) (/work/llvm/build/bin/clang-6.0+0x362aac6)
>   #2 0x00007f9193b78330 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x10330)
>   #3 0x00007f919276bc37 gsignal /build/eglibc-oGUzwX/eglibc-2.19/signal/../nptl/sysdeps/unix/sysv/linux/raise.c:56:0
>   #4 0x00007f919276f028 abort /build/eglibc-oGUzwX/eglibc-2.19/stdlib/abort.c:91:0
>   #5 0x00007f9192764bf6 __assert_fail_base /build/eglibc-oGUzwX/eglibc-2.19/assert/assert.c:92:0
>   #6 0x00007f9192764ca2 (/lib/x86_64-linux-gnu/libc.so.6+0x2fca2)
>   #7 0x0000000003d70eb9 (/work/llvm/build/bin/clang-6.0+0x3d70eb9)
>   #8 0x0000000003d6b00c llvm::RegBankSelect::applyMapping(llvm::MachineInstr&, llvm::RegisterBankInfo::InstructionMapping const&, llvm::SmallVectorImpl<llvm::RegBankSelect::RepairingPlacement>&) (/work/llvm/build/bin/clang-6.0+0x3d6b00c)
>   #9 0x0000000003d6b366 llvm::RegBankSelect::assignInstr(llvm::MachineInstr&) (/work/llvm/build/bin/clang-6.0+0x3d6b366)
>   #10 0x0000000003d6b7f1 llvm::RegBankSelect::runOnMachineFunction(llvm::MachineFunction&) (/work/llvm/build/bin/clang-6.0+0x3d6b7f1)
>   #11 0x0000000002d934c8 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/work/llvm/build/bin/clang-6.0+0x2d934c8)
>   #12 0x00000000030c998f llvm::FPPassManager::runOnFunction(llvm::Function&) (/work/llvm/build/bin/clang-6.0+0x30c998f)
>   #13 0x00000000030c9c53 llvm::FPPassManager::runOnModule(llvm::Module&) (/work/llvm/build/bin/clang-6.0+0x30c9c53)
>   #14 0x00000000030ca136 llvm::legacy::PassManagerImpl::run(llvm::Module&) (/work/llvm/build/bin/clang-6.0+0x30ca136)
>   #15 0x00000000037c3dcf clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (/work/llvm/build/bin/clang-6.0+0x37c3dcf)
>   #16 0x0000000003d421a0 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (/work/llvm/build/bin/clang-6.0+0x3d421a0)
>   #17 0x0000000004457376 clang::ParseAST(clang::Sema&, bool, bool) (/work/llvm/build/bin/clang-6.0+0x4457376)
>   #18 0x0000000003ca6ea0 clang::FrontendAction::Execute() (/work/llvm/build/bin/clang-6.0+0x3ca6ea0)
>   #19 0x0000000003c1fa31 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/work/llvm/build/bin/clang-6.0+0x3c1fa31)
>   #20 0x0000000003d3bf4b clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/work/llvm/build/bin/clang-6.0+0x3d3bf4b)
>   #21 0x0000000001f85629 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/work/llvm/build/bin/clang-6.0+0x1f85629)
>   #22 0x0000000001f83096 main (/work/llvm/build/bin/clang-6.0+0x1f83096)
>   #23 0x00007f9192756f45 __libc_start_main /build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:321:0
>   #24 0x0000000001f80029 _start (/work/llvm/build/bin/clang-6.0+0x1f80029)
>   Stack dump:
>   0.      Program arguments: /work/llvm/build/bin/clang-6.0 -cc1 -triple aarch64-arm-none-eabi -emit-obj -mrelax-all -disable-free -main-file-name test.c -mrelocation-model static -mthread-model posix -mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases -fuse-init-array -target-cpu generic -target-feature +neon -target-abi aapcs -fallow-half-arguments-and-returns -dwarf-column-info -debugger-tuning=gdb -coverage-notes-file /work/innovation/cctest/test.gcno -resource-dir /work/llvm/build/lib/clang/6.0.0 -O0 -fdebug-compilation-dir /work/innovation/cctest -ferror-limit 19 -fmessage-length 226 -fno-signed-char -fobjc-runtime=gcc -fdiagnostics-show-option -fcolor-diagnostics -mllvm -global-isel -mllvm -global-isel-abort=0 -o test.o -x c test.c 
>   1.      <eof> parser at end of file
>   2.      Code generation
>   3.      Running pass 'Function Pass Manager' on module 'test.c'.
>   4.      Running pass 'RegBankSelect' on function '@pass_f16'
>   clang-6.0: error: unable to execute command: Aborted (core dumped)
>   clang-6.0: error: clang frontend command failed due to signal (use -v to see invocation)
>   clang version 6.0.0 (ssh://olista01@ds-gerrit.euhpc.arm.com:29418/armcompiler/clang <ssh://olista01@ds-gerrit.euhpc.arm.com:29418/armcompiler/clang> aa2b9952ef98a5fe2d47384ef17106855b8bae51) (ssh://olista01@ds-gerrit.euhpc.arm.com:29418/armcompiler/llvm <ssh://olista01@ds-gerrit.euhpc.arm.com:29418/armcompiler/llvm> 29f89772107a79b5f2a816d4748ed9c19416c1b6)
>   Target: aarch64-arm-none-eabi
>   Thread model: posix
>   InstalledDir: /work/llvm/build/bin
>   clang-6.0: note: diagnostic msg: PLEASE submit a bug report to http://llvm.org/bugs/ <http://llvm.org/bugs/> and include the crash backtrace, preprocessed source, and associated run script.
>   clang-6.0: note: diagnostic msg:
>   ********************
>  
>   PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
>   Preprocessed source(s) and associated run script(s) are located at:
>   clang-6.0: note: diagnostic msg: /tmp/test-e06964.c
>   clang-6.0: note: diagnostic msg: /tmp/test-e06964.sh
>   clang-6.0: note: diagnostic msg:
>  
>   ********************
>  
> Oliver
>  
> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of Quentin Colombet via llvm-dev
> Sent: 13 November 2017 18:27
> To: Kristof Beyls
> Cc: llvm-dev; nd; Ahmed Bougacha; Justin Bogner; Aditya Nandakumar
> Subject: Re: [llvm-dev] [GlobalISel][AArch64] Toward flipping the switch for O0: Please give it a try!
>  
> Hi Kristof,
>  
>  
> On Nov 13, 2017, at 9:10 AM, Kristof Beyls <Kristof.Beyls at arm.com <mailto: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 <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/20171114/ee01f568/attachment.html>