[PATCH] [ARM] Implement eabi_attribute, cpu, and fpu directives.

Logan Chien tzuhsiang.chien at gmail.com
Fri Oct 25 10:07:28 PDT 2013



================
Comment at: test/CodeGen/ARM/2010-09-29-mc-asm-header-test.ll:81
@@ -80,3 +78,2 @@
-; V8-FPARMv8-NEON: .eabi_attribute 10, 7
 ; V8-FPARMv8-NEON: .eabi_attribute 12, 3
 
----------------
Richard Barton wrote:
> I understand that we are choosing not to emit the .eabi_attribute directive for Tag_FP_arch in favour of the .fpu directive because that's what GNU does, but we are also emitting the .eabi_attribute directive for Tag_Advanced_SIMD_arch at the same time. Is this just to be compatible with GNU as well?
> 
> 
I am removing most of the Tag_Advanced_SIMD_arch.  However, the following case can't be mapped to a single .fpu directive.

; RUN: llc < %s -mtriple=armv8-linux-gnueabi -mattr=-fp-armv8,-crypto | FileCheck %s --check-prefix=V8-NEON

That's why you still can see a special case in ARMAsmPrinter.cpp (Line:663)

================
Comment at: lib/Target/ARM/ARMAsmPrinter.cpp:663
@@ -870,6 +662,3 @@
     if (Subtarget->hasV8Ops())
-      AttrEmitter->EmitAttribute(ARMBuildAttrs::Advanced_SIMD_arch,
-                                 ARMBuildAttrs::AllowedNeonV8);
-    else
-      AttrEmitter->EmitAttribute(ARMBuildAttrs::Advanced_SIMD_arch,
-                                 ARMBuildAttrs::Allowed);
+      ATS.emitAttribute(ARMBuildAttrs::Advanced_SIMD_arch,
+                        ARMBuildAttrs::AllowNeonARMv8);
----------------
The special case which we keep emitting Tag_Advanced_SIMD_arch.

================
Comment at: test/CodeGen/ARM/2010-10-19-mc-elf-objheader.ll:33
@@ -8,3 +32,3 @@
 
 ; This tests that the extpected ARM attributes are emitted.
 ;
----------------
Richard Barton wrote:
> typo in comment
Thanks.  Will be fixed in next revision.

================
Comment at: test/CodeGen/ARM/2010-10-19-mc-elf-objheader.ll:22
@@ +21,3 @@
+; RUN:   | llvm-readobj -s -sd | FileCheck %s --check-prefix=V8-FPARMv8-NEON
+; RUN: llc < %s -mtriple=armv8-linux-gnueabi -filetype=obj \
+; RUN:   | llvm-readobj -s -sd \
----------------
Richard Barton wrote:
> This is a duplicate of line 9 above.
Thanks.  Will be removed.  I haven't notice this when I was copy the test command from 2010-09-29-mc-asm-header-test.ll.

================
Comment at: test/CodeGen/ARM/2010-10-19-mc-elf-objheader.ll:25
@@ +24,3 @@
+; RUN:   | FileCheck %s --check-prefix=V8-FPARMv8-NEON-CRYPTO
+; RUN: llc < %s -mtriple=armv7-linux-gnueabi -mcpu=cortex-a9 -filetype=obj \
+; RUN:   | llvm-readobj -s -sd \
----------------
Richard Barton wrote:
> This test generates identical attributes to line 7 above. It would be nice to see a test for a v7 core with VFPv4, so Cortex-A15.
Thanks.  Test for Cortex-A15 will be added in next revision of this patch.

================
Comment at: test/CodeGen/ARM/2010-10-19-mc-elf-objheader.ll:31
@@ -7,2 +30,3 @@
+; RUN:   | FileCheck %s --check-prefix=CORTEX-M0
 
 
----------------
Richard Barton wrote:
> It would be nice to get full coverage of the FPU options that you have touched, so:
>  * R4 - VFPv3_D16
>  * M4F AKA M4 - VFPv4_D16 (and also v7E-M)
>  * A15 - VFPv4+NEONv2 (and also without neon)
> 
> For the half-precision only variants, there is another build attribute that needs setting: Tag_VFP_HP_extension, but I accept that is beyond the original scope of this patch, so the test strings are ok without it for now.
Both cortex-m4 and cortex-a15 will be added to next revision of this patch.  However, it seems that LLVM can't recognize cortex-r4 as the mcpu option.  I will use cortex-r5 instead (p.s. According to the result, it seems that R5 is using VFPv3_D16 as well)

================
Comment at: lib/Target/ARM/ARMAsmPrinter.cpp:669
@@ +668,3 @@
+    else if (Subtarget->hasVFP4())
+      ATS.emitFPU(Subtarget->isFPOnlySP() ? ARM::VFPV4_D16 : ARM::VFPV4);
+    else if (Subtarget->hasVFP3())
----------------
After further testing, it seems that we have to use Subtarget->hasD16() instead.  However, I am not sure why was the old code using Subtarget->isFPOnlySP().  I will change this in next revision.  Any thoughts?


http://llvm-reviews.chandlerc.com/D1987



More information about the llvm-commits mailing list