Page MenuHomePhabricator

[zorg] Add AArch32 global-isel bots
ClosedPublic

Authored by maxim-kuvyrkov on Feb 9 2018, 3:44 AM.

Details

Summary

Add 32-bit ARMv7 and ARMv8 global-isel bots. These bots ensure that
global-isel does not regress at -O0.

Patch by Diana Picus.

Diff Detail

Repository
rL LLVM

Event Timeline

maxim-kuvyrkov created this revision.Feb 9 2018, 3:44 AM
rovka added inline comments.Feb 9 2018, 7:02 AM
buildbot/osuosl/master/config/builders.py
324 ↗(On Diff #133587)

Well, technically my patch was only for the ARMv7 part. I'm not sure how this one is supposed to work. What makes this AArch32? If we run on an armv8 machine, won't it by default use AArch64?

maxim-kuvyrkov added inline comments.Feb 9 2018, 8:06 AM
buildbot/osuosl/master/config/builders.py
324 ↗(On Diff #133587)

Docker containers are started on ARMv8 machine under "linux32" personality and with armhf ubuntu rootfs. This makes LLVM detect bot as armv8l-linux-gnueabihf .

rovka added inline comments.Feb 12 2018, 1:19 AM
buildbot/osuosl/master/config/builders.py
324 ↗(On Diff #133587)

Ah, ok. I'd like it better if we set the triple on the command line, since that would make things more explicit. Alternatively, it might be a good idea to build only the ARM target, and not AArch64, to make sure things blow up if this ends up on a misconfigured container in the future.

rengolin added inline comments.
buildbot/osuosl/master/config/builders.py
308 ↗(On Diff #133587)

you don't need all the -mllvm options, just O0 should be fine.

324 ↗(On Diff #133587)

I agree with @rovka, you shouldn't rely on the host to tell you what to run. We also want to test armv7 and armv8, so for now, setting the triple to armv8 should be fine, but we shouldn't disable the armv7 bot later in the future, as the code-gen path are different.

Now I remember why I chose to also force the CPU: because A15 stresses almost all relevant instructions form the ARMv7 set. If you don't choose anything you can end up picking A8 which will be the bare minimum.

In the ARMv8 case, you'll probably have A53, which will test more instructions, but will also stop testing ARMv7 specific paths. So, if we move to 32-bit emulation for ARMv7, we should have *both* ARMv7+A15 and ARMv8+A53/7.

Making the flags explicit also help reproducing builds and test, both for bisecting and investigating as well as re-building the container and getting a different behaviour (but still the same builds).

rovka added inline comments.Feb 13 2018, 5:56 AM
buildbot/osuosl/master/config/builders.py
308 ↗(On Diff #133587)

Yes, you do! This is ARM, not AArch64, we're not on-by-default yet. Please leave the -mllvm options there.

rengolin added inline comments.Feb 13 2018, 6:59 AM
buildbot/osuosl/master/config/builders.py
308 ↗(On Diff #133587)

D'oh! Ignore me!

maxim-kuvyrkov added inline comments.Feb 13 2018, 8:15 AM
buildbot/osuosl/master/config/builders.py
324 ↗(On Diff #133587)

Agree. I'm going to experiment with setting clang/LLVM default flags at cmake stage; if that doesn't work, we'll just pass triple via testsuite flags.

maxim-kuvyrkov accepted this revision.Feb 16 2018, 4:05 AM
maxim-kuvyrkov added inline comments.
buildbot/osuosl/master/config/builders.py
324 ↗(On Diff #133587)

I'm going to commit this as is, and connect to silent master. Then I'm going to experiment with triples and default arch flags in a future patch.
FWIW, there is extremely low chance of running any 32-bit armv8 bot in aarch64 world (and vice versa) due to how containers are configured.

This revision is now accepted and ready to land.Feb 16 2018, 4:05 AM
This revision was automatically updated to reflect the committed changes.