[ARM] Check for unaligned access via bitcasts

[samparker]

Summary

The load/store optimiser will convert sequential ldr/str instructions into ldrd/strd and these instructions cannot access unaligned memory.

When generating DSP instructions, we can use ldr/str instructions to load and store packed data but this means that it can be possible for those instructions to be accessing unaligned memory. This is fine for processors which support unaligned accesses, but it is not fine if these instructions are then optimised into ldrd/strd.

This patch modified ARMLoadStoreOpt pass to look through the pointer information to determine if the access size of the memory operation is safe for the optimisation. We now don't perform the transform if the access type is wider than the memory type.

This patch is to support the transformation being performed in D48128.

[efriedma]

In IR, if a pointer operand doesn't have the alignment given by the "align" annotation, the behavior is undefined. (If you don't explicitly specify the alignment, it defaults to the alignment specified in the DataLayout.) Similarly, in a MachineFunction, if a memory operation accesses an address which doesn't have the alignment given in the MMO, the behavior is undefined.

Looking at ARMParallelDSP::CreateSMLADCall in D48128, it looks like you forgot to set the alignment on the loads you're creating.

[samparker]

Many thanks Eli!

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

   if (MMO.isVolatile())
     return false;
 
   // Unaligned ldr/str is emulated by some kernels, but unaligned ldm/stm is
   // not.
   if (MMO.getAlignment() < 4)
     return false;
 
+  // Check that the pointer has not been cast up to a larger type, as this
+  // could create an unaligned access.
+  std::function<bool(const Value*)> IsAlignedPtr = [&](const Value *Ptr) {
+    auto *PtrTy = cast<PointerType>(Ptr->getType());
+    unsigned AccessSize = PtrTy->getElementType()->getScalarSizeInBits();
+    if (auto *Cast = dyn_cast<BitCastInst>(Ptr)) {
+      if (auto *CastedPtrTy = dyn_cast<PointerType>(Cast->getSrcTy())) {
+        unsigned MemSize = CastedPtrTy->getElementType()->getScalarSizeInBits();
+        if (isa<PointerType>(CastedPtrTy->getElementType()))
+          return IsAlignedPtr(Cast->getOperand(0));
+        else
+          return AccessSize <= MemSize;
+      }
+    }
+    return true;
+  };
+
+  // Ensure that the underlying memory is truly aligned. This ensures that we
+  // don't create create illegal accesses when the DSP instructions use LDR
+  // and STR to load and store packed data.
+  const MachinePointerInfo &PointerInfo = MMO.getPointerInfo();
+  if (!PointerInfo.V.isNull() && PointerInfo.V.is<const Value*>()) {
+    auto *PtrVal = PointerInfo.V.get<const Value*>();
+    if (auto *GEP = dyn_cast<GetElementPtrInst>(PtrVal)) {
+      if (!IsAlignedPtr(GEP->getPointerOperand()))
+        return false;
+    } else if (isa<BitCastInst>(PtrVal) && !IsAlignedPtr(PtrVal))
+      return false;
+  }
+
   // str <undef> could probably be eliminated entirely, but for now we just want
   // to avoid making a mess of it.
   // FIXME: Use str <undef> as a wildcard to enable better stm folding.
   if (MI.getOperand(0).isReg() && MI.getOperand(0).isUndef())
     return false;
 
   // Likewise don't mess with references to undefined addresses.
   if (MI.getOperand(1).isUndef())

test/CodeGen/ARM/ldrd-strd-unaligned.ll

+; RUN: llc -mtriple=thumbv7em -mcpu=cortex-m7 %s -o - | FileCheck %s
+; RUN: llc -mtriple=thumbv7 -mcpu=cortex-a8 %s -o - | FileCheck %s
+; RUN: llc -mtriple=thumbv8 -mcpu=cortex-a53 %s -o - | FileCheck %s
+
+; CHECK-LABEL: bitcast_ptr_ldr
+; CHECK-NOT: ldrd
+define i32 @bitcast_ptr_ldr(i16* %In) {
+entry:
+  %0 = bitcast i16* %In to i32*
+  %in.addr.0 = getelementptr inbounds i32, i32* %0, i32 0
+  %in.addr.1 = getelementptr inbounds i32, i32* %0, i32 1
+  %1 = load i32, i32* %in.addr.0, align 4
+  %2 = load i32, i32* %in.addr.1, align 4
+  %mul = mul i32 %1, %2
+  ret i32 %mul
+}
+
+; CHECK-LABEL: bitcast_gep_ldr
+; CHECK-NOT: ldrd
+define i32 @bitcast_gep_ldr(i16* %In) {
+entry:
+  %in.addr.0 = getelementptr inbounds i16, i16* %In, i32 0
+  %in.addr.1 = getelementptr inbounds i16, i16* %In, i32 2
+  %cast.0 = bitcast i16* %in.addr.0 to i32*
+  %cast.1 = bitcast i16* %in.addr.1 to i32*
+  %0 = load i32, i32* %cast.0, align 4
+  %1 = load i32, i32* %cast.1, align 4
+  %mul = mul i32 %0, %1
+  ret i32 %mul
+}
+
+; CHECK-LABEL: bitcast_ptr_str
+; CHECK-NOT: strd
+define void @bitcast_ptr_str(i32 %arg0, i32 %arg1, i16* %out) {
+entry:
+  %0 = bitcast i16* %out to i32*
+  %out.addr.0 = getelementptr inbounds i32, i32* %0, i32 0
+  %out.addr.1 = getelementptr inbounds i32, i32* %0, i32 1
+  store i32 %arg0, i32* %out.addr.0, align 4
+  store i32 %arg1, i32* %out.addr.1, align 4
+  ret void
+}
+
+; CHECK-LABEL: bitcast_gep_str
+; CHECK-NOT: strd
+define void @bitcast_gep_str(i32 %arg0, i32 %arg1, i16* %out) {
+entry:
+  %out.addr.0 = getelementptr inbounds i16, i16* %out, i32 0
+  %out.addr.1 = getelementptr inbounds i16, i16* %out, i32 2
+  %cast.0 = bitcast i16* %out.addr.0 to i32*
+  %cast.1 = bitcast i16* %out.addr.1 to i32*
+  store i32 %arg0, i32* %cast.0, align 4
+  store i32 %arg1, i32* %cast.1, align 4
+  ret void
+}