Relax unaligned access assertion when type is byte aligned

[dylanmckay]

Summary

This relaxes an assertion inside SelectionDAGBuilder which is overly
restrictive on targets which have no concept of alignment (such as AVR).

In these architectures, all types are aligned to 8-bits.

After this, LLVM will only assert that accesses are aligned on targets
which actually require alignment.

This patch follows from a discussion on llvm-dev a few months ago
http://llvm.1065342.n5.nabble.com/llvm-dev-Unaligned-atomic-load-store-td112815.html

[nemanjai]

A couple of general comments...
I'm not a fan of modifying the ABI of DataLayout for this. Seems that we can just compare the ABI alignment against 1 at the call site. But I also wonder if it would suffice to simply check that the load instruction's alignment is less than the ABI alignment for that type
if (I.getAlignment() < DL.getABITypeAlignment(Ty))

However, that makes the additional assumption that targets can handle atomic loads with an alignment that is the same as the ABI alignment for that type. I believe this is a safe assumption, but perhaps that should be checked with the target maintainers. In either case, I think a quick check with the target maintainers would be good.

[efriedma]

However, that makes the additional assumption that targets can handle atomic loads with an alignment that is the same as the ABI alignment for that type.

This is not a safe assumption. For example, 32-bit x86 supports atomic 64-bit loads with native alignment, but the ABI alignment of i64 is only 32 bits. (It might be technically possible to synthesize an unaligned atomic load on x86, but it's not something we try to do.)

include/llvm/IR/DataLayout.h

429 ↗

(On Diff #122601)

This helper is more likely to be confusing than actually helpful.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

4143

I would prefer to have a target hook here, so we don't end up miscompiling or generating some obscure instruction selection failure for a target that isn't expecting this.

[dylanmckay]

• Prefer explicitly-enabled TLI options rather than making assumptions based on data layout
• Remove misplaced functions on DataLayout

Thanks for the feedback @nemanjai and @efriedma

[dylanmckay]

Remove unneeded modifications to the diff left from the last update

[efriedma]

LGTM

include/llvm/CodeGen/TargetLowering.h

public:
   /// the backend supports.  Any smaller operations are widened in
   /// AtomicExpandPass.
   ///
   /// Note that *unlike* operations above the maximum size, atomic ops
   /// are still natively supported below the minimum; they just
   /// require a more complex expansion.
   unsigned getMinCmpXchgSizeInBits() const { return MinCmpXchgSizeInBits; }
 
+  /// Whether the target supports unaligned atomic operations.
+  bool supportsUnalignedAtomics() const { return SupportsUnalignedAtomics; }
+
   /// Whether AtomicExpandPass should automatically insert fences and reduce
   /// ordering for this atomic. This should be true for most architectures with
   /// weak memory ordering. Defaults to false.
   virtual bool shouldInsertFencesForAtomic(const Instruction *I) const {
     return false;
   }
 
   /// Perform a load-linked operation on Addr, returning a "Value *" with the
protected:
   /// Set the maximum atomic operation size supported by the
   /// backend. Atomic operations greater than this size (as well as
   /// ones that are not naturally aligned), will be expanded by
   /// AtomicExpandPass into an __atomic_* library call.
   void setMaxAtomicSizeInBitsSupported(unsigned SizeInBits) {
     MaxAtomicSizeInBitsSupported = SizeInBits;
   }
 
-  // Sets the minimum cmpxchg or ll/sc size supported by the backend.
+  /// Sets the minimum cmpxchg or ll/sc size supported by the backend.
   void setMinCmpXchgSizeInBits(unsigned SizeInBits) {
     MinCmpXchgSizeInBits = SizeInBits;
   }
 
+  /// Sets whether unaligned atomic operations are supported.
+  void setSupportsUnalignedAtomics(bool UnalignedSupported) {
+    SupportsUnalignedAtomics = UnalignedSupported;
+  }
+
 public:
   //===--------------------------------------------------------------------===//
   // Addressing mode description hooks (used by LSR etc).
   //
 
   /// CodeGenPrepare sinks address calculations into the same BB as Load/Store
   /// instructions reading the address. This allows as much computation as
   /// possible to be done in the address mode for that operand. This hook lets
private:
   /// Size in bits of the maximum atomics size the backend supports.
   /// Accesses larger than this will be expanded by AtomicExpandPass.
   unsigned MaxAtomicSizeInBitsSupported;
 
   /// Size in bits of the minimum cmpxchg or ll/sc operation the
   /// backend supports.
   unsigned MinCmpXchgSizeInBits;
 
+  /// This indicates if the target supports unaligned atomic operations.
+  bool SupportsUnalignedAtomics;
+
   /// If set to a physical register, this specifies the register that
   /// llvm.savestack/llvm.restorestack should save and restore.
   unsigned StackPointerRegisterToSaveRestore;
 
   /// This indicates the default register class to use for each ValueType the
   /// target supports natively.
   const TargetRegisterClass *RegClassForVT[MVT::LAST_VALUETYPE];
   unsigned char NumRegistersForVT[MVT::LAST_VALUETYPE];

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitAtomicLoad(const LoadInst &I) {
   AtomicOrdering Order = I.getOrdering();
   SyncScope::ID SSID = I.getSyncScopeID();
 
   SDValue InChain = getRoot();
 
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   EVT VT = TLI.getValueType(DAG.getDataLayout(), I.getType());
 
-  if (I.getAlignment() < VT.getStoreSize())
+  if (!TLI.supportsUnalignedAtomics() &&
+      I.getAlignment() < VT.getStoreSize())
     report_fatal_error("Cannot generate unaligned atomic load");
 
   MachineMemOperand *MMO =
       DAG.getMachineFunction().
       getMachineMemOperand(MachinePointerInfo(I.getPointerOperand()),
                            MachineMemOperand::MOVolatile |
                            MachineMemOperand::MOLoad,
                            VT.getStoreSize(),
                            I.getAlignment() ? I.getAlignment() :

lib/CodeGen/TargetLoweringBase.cpp

TargetLoweringBase::TargetLoweringBase(const TargetMachine &tm) : TM(tm) {
   PrefLoopAlignment = 0;
   GatherAllAliasesMaxDepth = 18;
   MinStackArgumentAlignment = 1;
   // TODO: the default will be switched to 0 in the next commit, along
   // with the Target-specific changes necessary.
   MaxAtomicSizeInBitsSupported = 1024;
 
   MinCmpXchgSizeInBits = 0;
+  SupportsUnalignedAtomics = false;
 
   std::fill(std::begin(LibcallRoutineNames), std::end(LibcallRoutineNames), nullptr);
 
   InitLibcallNames(LibcallRoutineNames, TM.getTargetTriple());
   InitCmpLibcallCCs(CmpLibcallCCs);
   InitLibcallCallingConvs(LibcallCallingConvs);
 }
 

lib/Target/AVR/AVRISelLowering.cpp

AVRTargetLowering::AVRTargetLowering(AVRTargetMachine &tm)
 
   // Compute derived properties from the register classes.
   computeRegisterProperties(tm.getSubtargetImpl()->getRegisterInfo());
 
   setBooleanContents(ZeroOrOneBooleanContent);
   setBooleanVectorContents(ZeroOrOneBooleanContent);
   setSchedulingPreference(Sched::RegPressure);
   setStackPointerRegisterToSaveRestore(AVR::SP);
+  setSupportsUnalignedAtomics(true);
 
   setOperationAction(ISD::GlobalAddress, MVT::i16, Custom);
   setOperationAction(ISD::BlockAddress, MVT::i16, Custom);
 
   setOperationAction(ISD::STACKSAVE, MVT::Other, Expand);
   setOperationAction(ISD::STACKRESTORE, MVT::Other, Expand);
   setOperationAction(ISD::DYNAMIC_STACKALLOC, MVT::i8, Expand);
   setOperationAction(ISD::DYNAMIC_STACKALLOC, MVT::i16, Expand);

test/CodeGen/AVR/unaligned-atomic-loads.ll

+; RUN: llc -mattr=addsubiw < %s -march=avr | FileCheck %s
+
+; This verifies that the middle end can handle an unaligned atomic load.
+;
+; In the past, an assertion inside the SelectionDAGBuilder would always
+; hit an assertion for unaligned loads and stores.
+
+%AtomicI16 = type { %CellI16, [0 x i8] }
+%CellI16 = type { i16, [0 x i8] }
+
+; CHECK-LABEL: foo
+; CHECK: ret
+define void @foo(%AtomicI16* %self) {
+start:
+  %a = getelementptr inbounds %AtomicI16, %AtomicI16* %self, i16 0, i32 0, i32 0
+  load atomic i16, i16* %a seq_cst, align 1
+  ret void
+}
+