[PATCH] [arm] Parse tag names for eabi_attribute directive.

Richard Barton richard.barton at arm.com
Tue Dec 17 09:28:02 PST 2013

  Hi Logan

  A couple of minor points in the comments, but fundamentally this patch looks great.


Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:8026
@@ -8025,3 +8025,3 @@
 /// parseDirectiveEabiAttr
 ///  ::= .eabi_attribute int, int
 bool ARMAsmParser::parseDirectiveEabiAttr(SMLoc L) {
Comment is out of date now.

Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:8079
@@ +8078,3 @@
+      .Case("Tag_FP_arch", ARMBuildAttrs::VFP_arch)
+      .Default(-1);
Tag_FP_arch is the new name for this attribute. I would prefer the enum member be renamed, and Tag_VFP_arch be the synonym.

Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:8074
@@ +8073,3 @@
+      CASE_EABI_ATTR_TAG(nodefaults)
+      CASE_EABI_ATTR_TAG(conformance)
+      CASE_EABI_ATTR_TAG(also_compatible_with)
Conformance should come after T2EE

Comment at: test/MC/ARM/directive-eabi_attribute-str.s:17
@@ +16,3 @@
+	.eabi_attribute Tag_CPU_arch, 0
+@ CHECK: .eabi_attribute 6, 0
I think this test would be more meaningful if we could use some real values in for the attribute. An AEABI build attribute with value 0 is normally semantically the same as not being present at all, so a valid implementation of the llvm-mc disassembly would be not to emit any .eabi_attribute at all. I suggest adding a meaningful value for each of the numeric ones as you have for the ones that take strings.


More information about the llvm-commits mailing list