Default to armv7 cpu for NaCl when march=arm

[dschuff]

Summary

When the arch is given as "arm" clang uses the default target CPU from
LLVM to determine what the real arch should be (i.e. "arm" becomes
"armv4t" because LLVM's getARMCPUForArch falls back to "arm7tdmi").
Default to "cortex-a9" so that we end up with "armv7" in clang.

the nacl-direct.c test in clang covers this case.

[rengolin]

The best way to fix this would be to either:

1. Use "armv7-*" as your triple in the first place, or
2. Set the CPU to what you really want, arch will follow, or
3. Get a NaCl toolchain class to do the actual transference

This patch "works" because the driver is dumb enough to go around in circles and pick the CPU from arch, arch from CPU and CPU from arch again, and this is just down in the middle. I believe the two existing hacks should go as soon as possible, and I wouldn't want to include a third one.

cheers,
--renato

lib/Support/Triple.cpp

1077 ↗

(On Diff #22601)

This fix is as wrong as the one above. You're trying to set to a random CPU because you want ARMv7, while it'd be much easier if you set "armv7" in the first place.

1078 ↗

(On Diff #22601)

That's unnecessary, since this is an ARM-only function.

1079 ↗

(On Diff #22601)

And cortex-a9 is not the best choice for almost anything. The base ARMv7 is Cortex-A8, and most NaCl devices will probably be running on Cortex-A15.

[dschuff]

OK, let me back up.
What I really want is that when I have a driver named "arm-nacl-clang" which results in an invocation like:
clang -target arm-nacl

then the underlying clang -cc1 invocation should be like
clang -cc1 -triple armv7--nacl-gnueabihf

(see line 70 of http://reviews.llvm.org/D8590#1a1873d0)

I don't mind doing general hack removal...
So it looks like clang's getARMCPUForMarch is the only user of this getARMCPUForArch. Should I just move this logic there first?

[dschuff]

replies inline but see earlier general comment

lib/Support/Triple.cpp

1078 ↗

(On Diff #22601)

I think it is what I want because if the user specifies something other than plain "arm" it should override this default.

1079 ↗

(On Diff #22601)

OK, noted.
All NaCl devices that I'm aware of are A15 but the base requirement for NaCl is specified as v7 + NEON.

[rengolin]

In D8589#146393, @dschuff wrote:

What I really want is that when I have a driver named "arm-nacl-clang" which results in an invocation like:
clang -target arm-nacl then the underlying clang -cc1 invocation should be like
clang -cc1 -triple armv7--nacl-gnueabihf

I thought so, and I think that's the broken part of it. GCC does it that way, but Clang driver doesn't, and hacks have been made to do so in this horrible way, and it's past time we fix this.

Why can't you have a driver named "armv7-nacl-gnueabihf-clang"?

So it looks like clang's getARMCPUForMarch is the only user of this getARMCPUForArch. Should I just move this logic there first?

I wish things were that simple. The last time I looked at that part of the code (you may imagined why) was at least one year ago. But what's still clear to me is that those functions hunt you on your sleep. :)

There was some refactoring a while ago, when most of them were moved to the same place (we had many copies of them, slightly different, as you would expect).

If you can't have the driver's name changed, then I should suggest changing the driver at the driver level. In Tools.cpp, freebsd, hexagon and others have created their own versions of drivers, maybe NaCl could do the same? I have to say my knowledge in the driver is old and distant now, but maybe someone else more acquainted with Clang could point you in the right direction.

[rengolin]

lib/Support/Triple.cpp

1078 ↗

(On Diff #22601)

I see what you mean.

1079 ↗

(On Diff #22601)

And that's one of the reasons fixing it here won't work. You can't possibly return here saying "A7+NEON", and neither A7 nor A15 demand NEON.

Clang's driver is a bit silly in that "gnueabihf" is what sets it +hard-fp, at least on Linux, so you'll have to make NaCl smarter about hard float.

[dschuff]

lib/Support/Triple.cpp

1079 ↗

(On Diff #22601)

Yeah, there's extra logic in D8590 that handles float-abi and target-abi. This part just handles switching the arch, so I do think that moving this into clang would at least keep the behavior the same while not having the weird cross-dependence.

[dschuff]

In theory I could just have armv7-nacl-gnueabihf-clang... But I'm trying to replace arm-nacl-gcc and make it as simple as possible on our existing users. especially those on Windows....

In D8590 I am creating my own version of the driver code as you suggest.

After looking a bit more, do think that, wrong or not, this code really does belong in clang and not in Triple though, and I think moving it there even without behavior change would be a strict improvement. I'll create a CL to move the logic to arm::getARMCPUForMArch there and delete this function.

[dschuff]

• move logic to fallback case

[dschuff]

In D8622 the discussion was that we can't just move this to clang, since there might be other frontends that use it. So we're back to this change. I moved the change down to the fallback case, which is a definite improvement and suits the original purpose of this change. I'm also still willing to do some separate cleanup, but I'm less clear on what should be done now; so I'm open to more specific suggestions on that.

[rengolin]

Seems a lot cleaner, thanks. Can you add a test for it on the unit tests that you removed from your last patch?

[dschuff]

• add unittest

[dschuff]

Done, thanks for the review

[rengolin]

LGTM, thanks!

llvm/trunk/lib/Support/Triple.cpp

case llvm::Triple::NetBSD:
     case llvm::Triple::GNUEABIHF:
     case llvm::Triple::GNUEABI:
     case llvm::Triple::EABIHF:
     case llvm::Triple::EABI:
       return "arm926ej-s";
     default:
       return "strongarm";
     }
+  case llvm::Triple::NaCl:
+    return "cortex-a8";
   default:
     switch (getEnvironment()) {
     case llvm::Triple::EABIHF:
     case llvm::Triple::GNUEABIHF:
       return "arm1176jzf-s";
     default:
       return "arm7tdmi";
     }
   }
 }

llvm/trunk/unittests/ADT/TripleTest.cpp

TEST(TripleTest, getARMCPUForArch) {
     llvm::Triple Triple("armv7s-apple-ios7");
     EXPECT_STREQ("swift", Triple.getARMCPUForArch());
   }
   {
     llvm::Triple Triple("armv7-apple-ios7");
     EXPECT_STREQ("cortex-a8", Triple.getARMCPUForArch());
     EXPECT_STREQ("swift", Triple.getARMCPUForArch("armv7s"));
   }
+  {
+    llvm::Triple Triple("arm--nacl");
+    EXPECT_STREQ("cortex-a8", Triple.getARMCPUForArch("arm"));
+  }
 }
 }
 
 TEST(TripleTest, NormalizeARM) {
   EXPECT_EQ("armv6--netbsd-eabi", Triple::normalize("armv6-netbsd-eabi"));
   EXPECT_EQ("armv7--netbsd-eabi", Triple::normalize("armv7-netbsd-eabi"));
   EXPECT_EQ("armv6eb--netbsd-eabi", Triple::normalize("armv6eb-netbsd-eabi"));
   EXPECT_EQ("armv7eb--netbsd-eabi", Triple::normalize("armv7eb-netbsd-eabi"));