[ARM] Fix ARM triple parsing.

[enefaim]

Summary

The triple parser should only accept existing architecture names
when the triple starts with armv, armebv, thumbv or thumbebv.

[rengolin]

Hi Gabor,

The idea is good, but I think we need a better implementation... Something that would be valid for other archs too.

As I see it, some arch strings are static (ex. "r600" or "hexagon"), and for them, a StringSwitch is the best way.

Others, like ARM ("armv*") and Intel ("i*", "x86*"), MIPS ("mips*") would better be separate on their own static inline functions returning the Triple::ArchType, not boolean. Your new function is good, but instead of boolean, change it to Triple::ArchType, and implement it for the others that have the same pattern.

cheers,
--renato

[echristo]

I'm not objecting to the idea, but I'm also not sure what you plan on gaining from this?

Also, tests?

-eric

[rengolin]

Eric,

From my point of view, the issue this solves is that now you can say "-triple armvelocipede" and it'll work. That's far from ideal.

I'd prefer to have a proper parser to get arch+cpu+features from triple/march/mcpu/mfpu/mtune options, but that'll require the TargetInfo parser, which will take a while. For now, this patch would at least cover the bogus cases in ARM.

I agree some tests are in order, but I believe we already have some.

Gabor,

Have you checked for current tests on that? Are they complete? Even if they are, we should have at least a few negative tests ("RUN: not" lines) to make sure we don't accept the things we say we won't.

cheers,
--renato

[enefaim]

Hi Renato,

Currently I see similar test cases in these Clang tests:
test/Driver/hexagon-toolchain.c
test/Driver/mips-abi.c

I plan to write similar Clang test cases checking for this error message
"error: unknown target triple 'armvelocipede--linux-gnueabi'"
when the ARM target triple is invalid, for example if we call clang like this:
"clang -target armvelocipede--linux-gnueabi"
and so forth with armebv, thumbv and thumbebv.

By the way, how does one submit patches that modify both clang and llvm?

Thanks,
Gabor

[rengolin]

Hi Gabor,

You need to create two reviews, one for each, and cross reference each other (in the text, writing DNNNN will link automatically).

However, LLVM changes normally have to be self-contained, so you should have tests in the LLVM side as well. I believe using llc for that should work.

When it gets to commit, you should commit the LLVM part first, and aim for it to pass even without Clang being updated, since not everyone uses clang. Clang commits can depend on LLVM ones, and that's why you need to commit to LLVM first.

cheers,
--renato

[enefaim]

Hi Renato,

I changed the ARM triple parsing function as you requested,
and added tests on LLVM side.
As for the triple starting with "i*", "x86*" and "mips*" I think
these cases can be easily handled by enumerating all possible
cases (there aren't so many as ARM cases), but if you still think
that these cases should be handled in their separate function
I will refactor the patch.

[rengolin]

lib/Support/Triple.cpp

270

Can't we just say:

.StartsWith("arm", parseARMArch(ArchName))
.StartsWith("thumb", parseARMArch(ArchName))

and add "arm64" to parseARMArch?

Maybe even adding "aarch64" to it, too, and having:

.StartsWith("arm", parseARMArch(ArchName))
.StartsWith("thumb", parseARMArch(ArchName))
.StartsWith("aarch64", parseARMArch(ArchName))

I wonder how Tim/James would feel about that...

[enefaim]

Hi Renato,

Simplified parseARMArch calls as requested.
Also added "arm64" and "aarch64".

[rengolin]

Thanks Gabor, I don't think we need tests of the correct archs, since they're used all over the place.

The implementation is looking a lot better, thanks!

-renato

[rengolin]

Committed in r222129.

Thanks!
--renato

lib/Support/Triple.cpp

     .Case("hsail", hsail)
     .Case("hsail64", hsail64)
     .Case("spir", spir)
     .Case("spir64", spir64)
     .Case("kalimba", kalimba)
     .Default(UnknownArch);
 }
 
+static Triple::ArchType parseARMArch(StringRef ArchName) {
+  size_t offset = StringRef::npos;
+  Triple::ArchType arch = Triple::UnknownArch;
+  bool isThumb = ArchName.startswith("thumb");
+
+  if (ArchName.equals("arm"))
+    return Triple::arm;
+  if (ArchName.equals("armeb"))
+    return Triple::armeb;
+  if (ArchName.equals("thumb"))
+    return Triple::thumb;
+  if (ArchName.equals("thumbeb"))
+    return Triple::thumbeb;
+  if (ArchName.equals("arm64") || ArchName.equals("aarch64"))
+    return Triple::aarch64;
+  if (ArchName.equals("aarch64_be"))
+    return Triple::aarch64_be;
+
+  if (ArchName.startswith("armv")) {
+    offset = 3;
+    arch = Triple::arm;
+  } else if (ArchName.startswith("armebv")) {
+    offset = 5;
+    arch = Triple::armeb;
+  } else if (ArchName.startswith("thumbv")) {
+    offset = 5;
+    arch = Triple::thumb;
+  } else if (ArchName.startswith("thumbebv")) {
+    offset = 7;
+    arch = Triple::thumbeb;
+  }
+  return StringSwitch<Triple::ArchType>(ArchName.substr(offset))
+    .Cases("v2", "v2a", isThumb ? Triple::UnknownArch : arch)
+    .Cases("v3", "v3m", isThumb ? Triple::UnknownArch : arch)
+    .Cases("v4", "v4t", arch)
+    .Cases("v5", "v5e", "v5t", "v5te", "v5tej", arch)
+    .Cases("v6", "v6j", "v6k", "v6m", arch)
+    .Cases("v6t2", "v6z", "v6zk", arch)
+    .Cases("v7", "v7a", "v7em", "v7l", arch)
+    .Cases("v7m", "v7r", "v7s", arch)
+    .Cases("v8", "v8a", arch)
+    .Default(Triple::UnknownArch);
+}
+
 static Triple::ArchType parseArch(StringRef ArchName) {
   return StringSwitch<Triple::ArchType>(ArchName)
     .Cases("i386", "i486", "i586", "i686", Triple::x86)
     // FIXME: Do we need to support these?
     .Cases("i786", "i886", "i986", Triple::x86)
     .Cases("amd64", "x86_64", "x86_64h", Triple::x86_64)
     .Case("powerpc", Triple::ppc)
     .Cases("powerpc64", "ppu", Triple::ppc64)
     .Case("powerpc64le", Triple::ppc64le)
-    .Case("aarch64", Triple::aarch64)
-    .Case("aarch64_be", Triple::aarch64_be)
-    .Case("arm64", Triple::aarch64)
-    .Cases("arm", "xscale", Triple::arm)
-    // FIXME: It would be good to replace these with explicit names for all the
-    // various suffixes supported.
-    .StartsWith("armv", Triple::arm)
-    .Case("armeb", Triple::armeb)
-    .StartsWith("armebv", Triple::armeb)
-    .Case("thumb", Triple::thumb)
-    .StartsWith("thumbv", Triple::thumb)
-    .Case("thumbeb", Triple::thumbeb)
-    .StartsWith("thumbebv", Triple::thumbeb)
+    .Case("xscale", Triple::arm)
+    .StartsWith("arm", parseARMArch(ArchName))
+    .StartsWith("thumb", parseARMArch(ArchName))
+    .StartsWith("aarch64", parseARMArch(ArchName))
     .Case("msp430", Triple::msp430)
     .Cases("mips", "mipseb", "mipsallegrex", Triple::mips)
     .Cases("mipsel", "mipsallegrexel", Triple::mipsel)
     .Cases("mips64", "mips64eb", Triple::mips64)
     .Case("mips64el", Triple::mips64el)
     .Case("r600", Triple::r600)
     .Case("hexagon", Triple::hexagon)
     .Case("s390x", Triple::systemz)

llvm/test/CodeGen/ARM/invalid-target.ll

+; RUN: not llc -mtriple armvinvalid-linux-gnueabi %s -o - 2>&1 | \
+; RUN: FileCheck %s --check-prefix=ARMVINVALID
+
+; RUN: not llc -mtriple armebvinvalid-linux-gnueabi %s -o - 2>&1 | \
+; RUN: FileCheck %s --check-prefix=ARMEBVINVALID
+
+; RUN: not llc -mtriple thumbvinvalid-linux-gnueabi %s -o - 2>&1 | \
+; RUN: FileCheck %s --check-prefix=THUMBVINVALID
+
+; RUN: not llc -mtriple thumbebvinvalid-linux-gnueabi %s -o - 2>&1 | \
+; RUN: FileCheck %s --check-prefix=THUMBEBVINVALID
+
+; RUN: not llc -mtriple thumbv2-linux-gnueabi %s -o - 2>&1 | \
+; RUN: FileCheck %s --check-prefix=THUMBV2
+
+; RUN: not llc -mtriple thumbv3-linux-gnueabi %s -o - 2>&1 | \
+; RUN: FileCheck %s --check-prefix=THUMBV3
+
+; RUN: not llc -mtriple arm64invalid-linux-gnu %s -o - 2>&1 | \
+; RUN: FileCheck %s --check-prefix=ARM64INVALID
+
+; RUN: not llc -mtriple aarch64invalid-linux-gnu %s -o - 2>&1 | \
+; RUN: FileCheck %s --check-prefix=AARCH64INVALID
+
+; ARMVINVALID: error: unable to get target for 'armvinvalid--linux-gnueabi'
+; ARMEBVINVALID: error: unable to get target for 'armebvinvalid--linux-gnueabi'
+; THUMBVINVALID: error: unable to get target for 'thumbvinvalid--linux-gnueabi'
+; THUMBEBVINVALID: error: unable to get target for 'thumbebvinvalid--linux-gnueabi'
+; THUMBV2: error: unable to get target for 'thumbv2--linux-gnueabi'
+; THUMBV3: error: unable to get target for 'thumbv3--linux-gnueabi'
+; ARM64INVALID: error: unable to get target for 'arm64invalid--linux-gnu'
+; AARCH64INVALID: error: unable to get target for 'aarch64invalid--linux-gnu'