[PATCH] D11470: [ARM] Define subtarget feature "strict-align"

[JF Bastien]

jfb added a subscriber: t.p.northover.

================
Comment at: test/CodeGen/ARM/2011-10-26-memset-inline.ll:2
@@ -1,3 +1,3 @@
 ; Make sure short memsets on ARM lower to stores, even when optimizing for size.
-; RUN: llc -march=arm < %s | FileCheck %s -check-prefix=CHECK-GENERIC
+; RUN: llc -march=arm -mattr=+strict-align < %s | FileCheck %s -check-prefix=CHECK-GENERIC
 ; RUN: llc -march=arm -mcpu=cortex-a8 < %s | FileCheck %s -check-prefix=CHECK-UNALIGNED
----------------
ahatanak wrote:
> jfb wrote:
> > ahatanak wrote:
> > > jfb wrote:
> > > > Doesn't this test's triple put it in the `hasV6Ops() && isTargetMachO()` category, which supports unaligned memory accesses?
> > > I'm not sure why it is this way, but it looks like thumbv7 triple doesn't result in HasV7Ops being set to true (see function ARM_MC::ParseARMTriple).
> > So this was a bug in the old code?
> > 
> > Does you clang change "fix" this bug?
> The clang patch does "fix" the bug: if the triple is thumbv7 and darwin, clang doesn't pass "+strict-align". 
> 
> I didn't realize this was a bug, but it appears that the changes made to ARM_MC::ParseARMTriple in r213367 weren't intentional based on what I see in this thread:
> 
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140630/224415.html
Ah OK, it would be good to get @t.p.northover or @rengolin to confirm.

================
Comment at: test/CodeGen/ARM/build-attributes.ll:62
@@ -61,2 +61,3 @@
 ; RUN: llc < %s -mtriple=armv7-linux-gnueabi -mcpu=cortex-a17 -enable-sign-dependent-rounding-fp-math | FileCheck %s --check-prefix=DYN-ROUNDING
 ; RUN: llc < %s -mtriple=thumbv6m-linux-gnueabi -mcpu=cortex-m0 | FileCheck %s --check-prefix=CORTEX-M0
+; RUN: llc < %s -mtriple=thumbv6m-linux-gnueabi -mcpu=cortex-m0 -mattr=+strict-align  -enable-unsafe-fp-math -disable-fp-elim -enable-no-infs-fp-math -enable-no-nans-fp-math -fp-contract=fast | FileCheck %s --check-prefix=CORTEX-M0-FAST
----------------
ahatanak wrote:
> jfb wrote:
> > ahatanak wrote:
> > > jfb wrote:
> > > > This v6m needs strict-align?
> > > Don't we need strict-align since no v6M cores support unaligned memory access?
> > IIUC you should add `-mattr=+strict-align` here. After your fix clang will do it automatically, but llc won't so you need the flag.
> Ah, I was looking at a completely different place. We need -mattr=+strict-align here. The test was passing because it wasn't checking the value of Tag_CPU_unaligned_access.
So you'll update the patch?

================
Comment at: test/CodeGen/ARM/build-attributes.ll:129
@@ -130,1 +128,3 @@
+; RUN: llc < %s -mtriple=armv8.1a-none-linux-gnueabi | FileCheck %s --check-prefix=NO-STRICT-ALIGN
+; RUN: llc < %s -mtriple=armv8.1a-none-linux-gnueabi -mattr=+strict-align | FileCheck %s --check-prefix=STRICT-ALIGN
 ; RUN: llc < %s -mtriple=armv8.1a-none-linux-gnueabi | FileCheck %s --check-prefix=NO-STRICT-ALIGN
----------------
ahatanak wrote:
> jfb wrote:
> > ahatanak wrote:
> > > jfb wrote:
> > > > The current code didn't affect v8, so this seems wrong. I think it should fix v8 to use the same flags, though.
> > > For the first test, I removed -arm-no-strict-align because v8 does allow unaligned access, and for the second test, I replaced -arm-strict-align with -mattr=+strict-align.
> > The llc flag `-arm-no-strict-align` and `-arm-strict-align` don't do anything for v8.
> > With your current code, `-mattr=+strict-align` also doesn't do anything for v8.
> > 
> > v8 does have `-aarch64-strict-align` and `-aarch64-no-strict-align`, which I suggested you change to be consistent with the ARM change.
> > 
> > This line went from being misguided to being differently misguided. I'd rather fix v8 to act the same as ARM :-)
> I'm not sure I understood your comment, but I'm seeing a difference in the output when I use -arm-strict-align. Tag_CPU_unaligned_access is 1 when I use -arm-strict-align, but when I use -arm-no-strict-align or use no command line options for alignment, it is 0.
> 
> .eabi_attribute	34, 1	@ Tag_CPU_unaligned_access
Ah! `armv8.1a` is aarch32, not aarch64. So yes, this test is affected by your change.


http://reviews.llvm.org/D11470