This is an archive of the discontinued LLVM Phabricator instance.

[ARM. AArch64]Handle generic cpus in the gcc-compatible manner (llvm part)
AbandonedPublic

Authored by rengolin on Jul 30 2015, 2:58 AM.

Details

Summary

The patch mimics GCC's behaviour for the following:

  • default -march is "armv4t"
  • if an architecture is targeted, just a "generic" cpu is used with minimal sane set of features, instead of some specific real default cpu name.
  • LLVM's specific exceptions:
    • for specific vendors, OSes, environments, or arches - add the minimally required features, or even pick a real cpu.
  • build attributes for generic cpus:
    • asm: ".arch armv<ArchName>"
    • ELF: "Tag_CPU_name = "<ArchName>""
  • default "-mfloat-abi" is "soft".
  • if case of "-mfloat-abi=hard", and no "-mfpu" provided -> default "vfp2" is used.

Some revealed LLVM bugs had to be fixed along the patch preparation, because of test fails.

  • Crypto feature did not include Neon feature. This was not visible, because Neon was always on.
  • Despite requirements for minimal cpus were provided in Triple::getARMCPUForArch(), they did not always work.

Because of the latter fix and default arch upgrade, tests for codegen were also adjusted.

Diff Detail

Repository
rL LLVM

Event Timeline

vsukharev updated this revision to Diff 31009.Jul 30 2015, 2:58 AM
vsukharev retitled this revision from to Handle generic cpus in the gcc-compatible manner (llvm part).
vsukharev updated this object.
vsukharev added reviewers: t.p.northover, rengolin.
vsukharev set the repository for this revision to rL LLVM.
vsukharev retitled this revision from Handle generic cpus in the gcc-compatible manner (llvm part) to [ARM. AArch64]Handle generic cpus in the gcc-compatible manner (llvm part).Jul 30 2015, 2:59 AM
jfb added inline comments.Jul 30 2015, 8:07 AM
lib/Support/Triple.cpp
1330

NaCl mandates that the CPU be at least v7 with VFPv3 and NEON, so I think we want to have "cortex-a8" as it was before. NaCl does support integer division when available, so we could handle that too, but it doesn't currently handle features that are new in v8.

vsukharev added a comment.EditedJul 30 2015, 11:27 AM

Hi, it's great to have an NaCl expert around.
About architectures, lower than v7:
If armv6 or below is explicitly asked, should we emit a fault like "NaCl requires at least v7 architecture"?

Hmm, let's check, which CPU is in current output of the following line?

clang  -target armv6-unknown-nacl-gnueabihf -### /work/llvm/tools/clang/test/Driver/arm-alignment.c

I got "-target-cpu" "arm1136jf-s"...

  1. When I left only "cortex-a8", one test failed: tools/clang/test/Driver/arm-alignment.c, line 59 - it assumes CPU might be lower than v7, as you said...
// RUN: %clang -target armv6-unknown-nacl-gnueabihf -### %s 2> %t
// RUN: FileCheck --check-prefix=CHECK-ALIGNED-ARM < %t %s
// CHECK-ALIGNED-ARM: "-target-feature" "+strict-align"
  1. Whilst tools/clang/lib/Driver/Tools.cpp, line 834 explicitly prohibits such an emission for cortex-a8, that is of v7 architecture.
else if (Triple.isOSLinux() || Triple.isOSNaCl()) {
      if (VersionNum < 7)
        Features.push_back("+strict-align");

This snippet also implies NaCl supports CPUs lower than v7

If "armv6-unknown-nacl-gnueabihf " must have not "arm1136jf-s", but "cortex-a8" (that is quite weird for me, because it's not v6),
so either one of the following needs correction

  • lib/Driver/Tools.cpp, line 834
  • test/Driver/arm-alignment.c, line 59

What's your vision?

About v8 architecture:
thank you for the information on NaCl, I was in doubt, which one of Cortex-A53 or Cortex-A8 should I use. Based on absence of relevant tests, I decided that NaCl doesn't support v8, and any decision would be right.
In next patch revision I could leave Cortex-A8 for v8 as well, with an appropriate comment.

jfb added a comment.Jul 30 2015, 1:07 PM

Hi, it's great to have an NaCl expert around.
About architectures, lower than v7:
If armv6 or below is explicitly asked, should we emit a fault like "NaCl requires at least v7 architecture"?

Hmm, let's check, which CPU is in current output of the following line?

clang  -target armv6-unknown-nacl-gnueabihf -### /work/llvm/tools/clang/test/Driver/arm-alignment.c

I got "-target-cpu" "arm1136jf-s"...

  1. When I left only "cortex-a8", one test failed: tools/clang/test/Driver/arm-alignment.c, line 59 - it assumes CPU might be lower than v7, as you said...
// RUN: %clang -target armv6-unknown-nacl-gnueabihf -### %s 2> %t
// RUN: FileCheck --check-prefix=CHECK-ALIGNED-ARM < %t %s
// CHECK-ALIGNED-ARM: "-target-feature" "+strict-align"
  1. Whilst tools/clang/lib/Driver/Tools.cpp, line 834 explicitly prohibits such an emission for cortex-a8, that is of v7 architecture.
else if (Triple.isOSLinux() || Triple.isOSNaCl()) {
      if (VersionNum < 7)
        Features.push_back("+strict-align");

This snippet also implies NaCl supports CPUs lower than v7

If "armv6-unknown-nacl-gnueabihf " must have not "arm1136jf-s", but "cortex-a8" (that is quite weird for me, because it's not v6),
so either one of the following needs correction

  • lib/Driver/Tools.cpp, line 834
  • test/Driver/arm-alignment.c, line 59

What's your vision?

Agreed with what you suggest: if a developer asks for v6 on NaCl then let's honor this (since it's mostly a subset of what v7 has), instead of bailing out. So I'm now OK with your change :-)

Just to be sure: if the user doesn't specify v7 or v6 (e.g. arm-unknown-nacl), what do they get? v7 and cortex-a8 should be what they get.

Also note: NaCl doesn't support Thumb or Thumb2.

About v8 architecture:
thank you for the information on NaCl, I was in doubt, which one of Cortex-A53 or Cortex-A8 should I use. Based on absence of relevant tests, I decided that NaCl doesn't support v8, and any decision would be right.
In next patch revision I could leave Cortex-A8 for v8 as well, with an appropriate comment.

I think it may be better to error out if v8 is specified with NaCl.

jfb added a subscriber: dschuff.Jul 30 2015, 1:07 PM
rengolin edited edge metadata.Jul 30 2015, 1:38 PM

I haven't looked at the code entirely, but the way we're dealing with default features is by getting the CPU name and then its features.

To get a base default, just make the default arch ARMv4T and you got it. I can't see why you would want to add yet another way to find the default features to the existing one.

Also, the CPU name in the target-features is an internal thing to Clang/LLVM, and I don't think we should make it identical to GCC. There is no way that any GNU tool will understand those options, so it actually doesn't matter.

Having a named CPU, not just the correct set of target-features, is a good way to test, and also helps on constructing build attributes, and interacting with GNU tools at a later stage without relying on the fact that they understand the "generic" with the same features as we do.

At least we know that the specific CPU features are close enough to work well for all these years. I wouldn't bet on the "generic" model to be accurate, since we never used that. Ie. I wouldnt' just change it in production without a large system validation to make sure it does work well with all LLVM and GNU tools.

Just to be clear, the target parser code desperately needs to be table-genned. Adding more and more complex logic to it will make it harder, and eventually impossible to generate code at table-gen time.

rengolin requested changes to this revision.Jul 31 2015, 4:31 AM
rengolin edited edge metadata.

Hi Vladmir,

There are too many issues with this patch to consider merging it at all.

This is a big decision that affects all users, OS distributions, IDEs, integrated products, etc. You cannot send a merge request of that magnitude without getting consensus from the community first. I'm against such a change, so if you really want to see this through, you'll have to gain enough momentum in the rest of the community first. That includes Windows, Darwin, FreeBSD and Linux folks, as well as at least Apple and Google, which ship products with LLVM-ARM, and other tools, like LLD, LLDB, etc, which will invariably be affected, at least on their command-line interface.

This request also has too many changes in one go, and most of them reach further than you can see. It'll be virtually impossible to steer around problems that are found by validation or trial and error outside of the buildbot arena. And that's only after all the buildbots have calmed down, which due to the magnitude of this patch, I believe it'll take a while, and many patches, and mane reverts. We tend to avoid that kind of situation as much as possible, because we rely on the bots to be green, and having them unstable for a week or so means other failures will be harder to pinpoint, bisect, and fix.

As it stands, I'm rejecting this patch wholesale on the grounds of lack of consensus, proper testing and overall design decisions.

cheers,
-renato

lib/Support/TargetParser.cpp
425

This will have consequences in Clang. Have you ran "make check-all" with clang builtin?

lib/Support/Triple.cpp
1330

This is not true just for NaCl, but for almost every architecture. Your change makes no sense in the grand scheme of things.

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
142

This change makes no sense. If you want to make it proper, you'll have to iterate through the target features, either from the user info (command-line options, not available here yet), or from the table-gen description (via hasFeatureXXX methods).

This change is just making it hard-coded in a different way. Not worth the change.

This revision now requires changes to proceed.Jul 31 2015, 4:31 AM
rengolin commandeered this revision.Jun 27 2016, 6:44 AM
rengolin edited reviewers, added: vsukharev; removed: rengolin.
rengolin abandoned this revision.Jun 27 2016, 6:44 AM