[PATCH] D152790: [ARM] Fix codegen of unaligned volatile load/store of i64

[Maurice Heumann via Phabricator via llvm-commits]

momo5502 updated this revision to Diff 531670.
momo5502 added a comment.

In D152790#4419917 <https://reviews.llvm.org/D152790#4419917>, @efriedma wrote:

> I have no idea why the ARMLoadStoreOptimizer code was written that way; maybe fix that too, for the sake of clarity.

I adjusted it here to Align(4).

I also tried to adjust the ARMLoadStoreOptimizer and it turns out that getABITypeAlign of i64 is 8 and not 4,. That seems wrong in case of aligning loads. Therefore your comment is valid and it should be fixed there too. However, that breaks quite a bunch of tests and I feel this exceeds the scope of this change. I recommend creating an issue for that.

@dmgreen, concerning `allowsUnalignedMem`, I'm not sure how to proceed with that. Only applying the alignment check form this change when strict-alignment is enabled causes the instructions in this tests to be lowered to 16 one-byte loads and stores. However, 2 four-byte loads seem perfectly fine to me, at least on the system where I encountered this issue. To me, not taking `allowsUnalignedMem` into consideration seems fine. What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152790/new/

https://reviews.llvm.org/D152790

Files:
  llvm/lib/Target/ARM/ARMISelLowering.cpp
  llvm/test/CodeGen/ARM/i64_volatile_load_store.ll


Index: llvm/test/CodeGen/ARM/i64_volatile_load_store.ll
===================================================================
--- llvm/test/CodeGen/ARM/i64_volatile_load_store.ll
+++ llvm/test/CodeGen/ARM/i64_volatile_load_store.ll
@@ -5,6 +5,9 @@
 @x = common dso_local global i64 0, align 8
 @y = common dso_local global i64 0, align 8
 
+ at x_unaligned = common dso_local global i64 0, align 1
+ at y_unaligned = common dso_local global i64 0, align 1
+
 define void @test() {
 entry:
 ; CHECK-LABEL: test:
@@ -29,6 +32,34 @@
   ret void
 }
 
+define void @test_unaligned() {
+entry:
+; CHECK-LABEL: test_unaligned:
+; CHECK-ARMV5TE:      ldr [[ADDR0:r[0-9]+]]
+; CHECK-ARMV5TE-NEXT: ldr [[ADDR1:r[0-9]+]]
+; CHECK-ARMV5TE-NEXT: ldr [[R1:r[0-9]+]], [[[ADDR0]]]
+; CHECK-ARMV5TE-NEXT: ldr [[R0:r[0-9]+]], [[[ADDR0]], #4]
+; CHECK-ARMV5TE-NEXT: str [[R0]], [[[ADDR1]], #4]
+; CHECK-ARMV5TE-NEXT: str [[R1]], [[[ADDR1]]]
+; CHECK-T2:           movw [[ADDR0:r[0-9]+]], :lower16:x_unaligned
+; CHECK-T2-NEXT:      movw [[ADDR1:r[0-9]+]], :lower16:y_unaligned
+; CHECK-T2-NEXT:      movt [[ADDR0]], :upper16:x_unaligned
+; CHECK-T2-NEXT:      movt [[ADDR1]], :upper16:y_unaligned
+; CHECK-T2-NEXT:      ldr [[R1]], [[[ADDR0]]]
+; CHECK-T2-NEXT:      ldr [[R0]], [[[ADDR0]], #4]
+; CHECK-T2-NEXT:      str [[R0]], [[[ADDR1]], #4]
+; CHECK-T2-NEXT:      str [[R1]], [[[ADDR1]]]
+; CHECK-ARMV4T:       ldr [[ADDR0:r[0-9]+]]
+; CHECK-ARMV4T-NEXT:  ldr [[ADDR1:r[0-9]+]]
+; CHECK-ARMV4T-NEXT:  ldr [[R1:r[0-9]+]], [[[ADDR0]]]
+; CHECK-ARMV4T-NEXT:  ldr [[R0:r[0-9]+]], [[[ADDR0]], #4]
+; CHECK-ARMV4T-NEXT:  str [[R0]], [[[ADDR1]], #4]
+; CHECK-ARMV4T-NEXT:  str [[R1]], [[[ADDR1]]]
+  %0 = load volatile i64, ptr @x_unaligned, align 1
+  store volatile i64 %0, ptr @y_unaligned, align 1
+  ret void
+}
+
 define void @test_offset() {
 entry:
 ; CHECK-LABEL: test_offset:
Index: llvm/lib/Target/ARM/ARMISelLowering.cpp
===================================================================
--- llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -10080,7 +10080,8 @@
   assert(LD->isUnindexed() && "Loads should be unindexed at this point.");
 
   if (MemVT == MVT::i64 && Subtarget->hasV5TEOps() &&
-      !Subtarget->isThumb1Only() && LD->isVolatile()) {
+      !Subtarget->isThumb1Only() && LD->isVolatile() &&
+      LD->getAlign() >= Align(Subtarget->hasV6Ops() ? 4 : 8)) {
     SDLoc dl(N);
     SDValue Result = DAG.getMemIntrinsicNode(
         ARMISD::LDRD, dl, DAG.getVTList({MVT::i32, MVT::i32, MVT::Other}),
@@ -10137,7 +10138,8 @@
   assert(ST->isUnindexed() && "Stores should be unindexed at this point.");
 
   if (MemVT == MVT::i64 && Subtarget->hasV5TEOps() &&
-      !Subtarget->isThumb1Only() && ST->isVolatile()) {
+      !Subtarget->isThumb1Only() && ST->isVolatile() &&
+      ST->getAlign() >= Align(Subtarget->hasV6Ops() ? 4 : 8)) {
     SDNode *N = Op.getNode();
     SDLoc dl(N);
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D152790.531670.patch
Type: text/x-patch
Size: 2925 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230615/b173d200/attachment.bin>