Add address space argument to allowsUnalignedMemoryAccess.

[arsenm]

Summary

On R600, some address spaces have more strict alignment requirements than others.

Updating the uses to pass the address space will be a separate patch.

[arsenm]

Attach right version of patch that swaps order of arguments

[arsenm]

Ping

[qcolombet]

Hi Matt,

The overall fix goes in the right direction, but you have a couple of typos in the subclasses when you overload the allowsUnalignedMemoryAccesses.

I am not in favor of the change of order for the Fast argument in the modified API.
Indeed, because of that change, every call that uses the Fast argument will need to be updated (like you did for the ARM backend).
Therefore, I would prefer to have:
virtual bool allowsUnalignedMemoryAccesses(EVT, bool * /*Fast*/ = 0, unsigned AddrSpace = 0) const

That said, I understand the form you are proposing is more convenient for your target, thus if the majority agrees for the order change, I can cope with that :).

Thanks,

Quentin

include/llvm/Target/TargetLowering.h

715

Since you are updating the command, you could fix the typo here :).
The sentence is not finished here, thus the period should be removed.

717

if we decide to go with your API change:
s/second/third/

719

I believe.
s/It’s/Its/

lib/Target/Mips/MipsSEISelLowering.cpp

249

Same here.

lib/Target/Mips/MipsSEISelLowering.h

34

This prototype does not match the base class.
You’ve inverted the order of bool and unsigned.

[arsenm]

The reason I swapped the order was because the address space could be necessary for correctness, but Fast in principle could always be optional. When updating the uses of it, more places required throwing in a null pointer for Fast just to specify the address space rather than the other way around.

[arsenm]

By correctness I mean the actual answer instead of the conservatively correct false.

[qcolombet]

Fair enough.

Still there are a bunch of places where you forgot to swap the arguments.
See my inlined comments.

Thanks,
-Quentin

[arsenm]

Fix typos and Mips's implementation being swapped

[qcolombet]

Hi Matt,

This is looking good. There is just one little quirk that I mentioned in the inline comments.

With this change, LGTM.

Thanks,
Quentin

lib/Target/Mips/MipsSEISelLowering.h

34

You’ll need to set the default value for Fast as well.

[arsenm]

r200887

[qcolombet]

It landed a while ago: r200887

include/llvm/Target/TargetLowering.h

 
   /// \brief Determine if the target supports unaligned memory accesses.
   ///
-  /// This function returns true if the target allows unaligned memory accesses.
-  /// of the specified type. If true, it also returns whether the unaligned
-  /// memory access is "fast" in the second argument by reference. This is used,
-  /// for example, in situations where an array copy/move/set is converted to a
-  /// sequence of store operations. It's use helps to ensure that such
-  /// replacements don't generate code that causes an alignment error (trap) on
-  /// the target machine.
-  virtual bool allowsUnalignedMemoryAccesses(EVT, bool * /*Fast*/ = 0) const {
+  /// This function returns true if the target allows unaligned memory accesses
+  /// of the specified type in the given address space. If true, it also returns
+  /// whether the unaligned memory access is "fast" in the third argument by
+  /// reference. This is used, for example, in situations where an array
+  /// copy/move/set is converted to a sequence of store operations. Its use
+  /// helps to ensure that such replacements don't generate code that causes an
+  /// alignment error (trap) on the target machine.
+  virtual bool allowsUnalignedMemoryAccesses(EVT,
+                                             unsigned AddrSpace = 0,
+                                             bool * /*Fast*/ = 0) const {
     return false;
   }
 

lib/Target/ARM/ARMISelLowering.h

     /// allowsUnalignedMemoryAccesses - Returns true if the target allows
     /// unaligned memory accesses of the specified type. Returns whether it
     /// is "fast" by reference in the second argument.
-    virtual bool allowsUnalignedMemoryAccesses(EVT VT, bool *Fast) const;
+    virtual bool allowsUnalignedMemoryAccesses(EVT VT, unsigned AddrSpace,
+                                               bool *Fast) const;
 
     virtual EVT getOptimalMemOpType(uint64_t Size,
                                     unsigned DstAlign, unsigned SrcAlign,

lib/Target/ARM/ARMISelLowering.cpp

   return (VT == MVT::f32) && (Opc == ISD::LOAD || Opc == ISD::STORE);
 }
 
-bool ARMTargetLowering::allowsUnalignedMemoryAccesses(EVT VT, bool *Fast) const {
+bool ARMTargetLowering::allowsUnalignedMemoryAccesses(EVT VT, unsigned,
+                                                      bool *Fast) const {
   // The AllowsUnaliged flag models the SCTLR.A setting in ARM cpus
   bool AllowsUnaligned = Subtarget->allowsUnalignedMem();
 

     bool Fast;
     if (Size >= 16 &&
         (memOpAlign(SrcAlign, DstAlign, 16) ||
-         (allowsUnalignedMemoryAccesses(MVT::v2f64, &Fast) && Fast))) {
+         (allowsUnalignedMemoryAccesses(MVT::v2f64, 0, &Fast) && Fast))) {
       return MVT::v2f64;
     } else if (Size >= 8 &&
                (memOpAlign(SrcAlign, DstAlign, 8) ||
-                (allowsUnalignedMemoryAccesses(MVT::f64, &Fast) && Fast))) {
+                (allowsUnalignedMemoryAccesses(MVT::f64, 0, &Fast) && Fast))) {
       return MVT::f64;
     }
   }

lib/Target/Mips/Mips16ISelLowering.h

   public:
     explicit Mips16TargetLowering(MipsTargetMachine &TM);
 
-    virtual bool allowsUnalignedMemoryAccesses(EVT VT, bool *Fast) const;
+    virtual bool allowsUnalignedMemoryAccesses(EVT VT, unsigned AddrSpace,
+                                               bool *Fast) const;
 
     virtual MachineBasicBlock *
     EmitInstrWithCustomInserter(MachineInstr *MI, MachineBasicBlock *MBB) const;

lib/Target/Mips/Mips16ISelLowering.cpp

 }
 
 bool
-Mips16TargetLowering::allowsUnalignedMemoryAccesses(EVT VT, bool *Fast) const {
+Mips16TargetLowering::allowsUnalignedMemoryAccesses(EVT VT,
+                                                    unsigned,
+                                                    bool *Fast) const {
   return false;
 }
 

lib/Target/Mips/MipsSEISelLowering.h

     void addMSAFloatType(MVT::SimpleValueType Ty,
                          const TargetRegisterClass *RC);
 
-    virtual bool allowsUnalignedMemoryAccesses(EVT VT, bool *Fast) const;
+    virtual bool allowsUnalignedMemoryAccesses(EVT VT, unsigned AS = 0,
+                                               bool *Fast) const LLVM_OVERRIDE;
 
     virtual SDValue LowerOperation(SDValue Op, SelectionDAG &DAG) const;
 

lib/Target/Mips/MipsSEISelLowering.cpp

 }
 
 bool
-MipsSETargetLowering::allowsUnalignedMemoryAccesses(EVT VT, bool *Fast) const {
+MipsSETargetLowering::allowsUnalignedMemoryAccesses(EVT VT,
+                                                    unsigned,
+                                                    bool *Fast) const {
   MVT::SimpleValueType SVT = VT.getSimpleVT().SimpleTy;
 
   switch (SVT) {

lib/Target/PowerPC/PPCISelLowering.h

 
     /// Is unaligned memory access allowed for the given type, and is it fast
     /// relative to software emulation.
-    virtual bool allowsUnalignedMemoryAccesses(EVT VT, bool *Fast = 0) const;
+    virtual bool allowsUnalignedMemoryAccesses(EVT VT,
+                                               unsigned AddrSpace,
+                                               bool *Fast = 0) const;
 
     /// isFMAFasterThanFMulAndFAdd - Return true if an FMA operation is faster
     /// than a pair of fmul and fadd instructions. fmuladd intrinsics will be

lib/Target/PowerPC/PPCISelLowering.cpp

 }
 
 bool PPCTargetLowering::allowsUnalignedMemoryAccesses(EVT VT,
+                                                      unsigned,
                                                       bool *Fast) const {
   if (DisablePPCUnaligned)
     return false;

lib/Target/R600/SIISelLowering.h

 
 public:
   SITargetLowering(TargetMachine &tm);
-  bool allowsUnalignedMemoryAccesses(EVT  VT, bool *IsFast) const;
+  bool allowsUnalignedMemoryAccesses(EVT VT, unsigned AS, bool *IsFast) const;
   virtual bool shouldSplitVectorElementType(EVT VT) const;
 
   SDValue LowerFormalArguments(SDValue Chain, CallingConv::ID CallConv,

lib/Target/R600/SIISelLowering.cpp

 //===----------------------------------------------------------------------===//
 
 bool SITargetLowering::allowsUnalignedMemoryAccesses(EVT  VT,
+                                                     unsigned AddrSpace,
                                                      bool *IsFast) const {
   // XXX: This depends on the address space and also we may want to revist
   // the alignment values we specify in the DataLayout.

lib/Target/SystemZ/SystemZISelLowering.h

   virtual bool isFPImmLegal(const APFloat &Imm, EVT VT) const LLVM_OVERRIDE;
   virtual bool isLegalAddressingMode(const AddrMode &AM, Type *Ty) const
      LLVM_OVERRIDE;
-  virtual bool allowsUnalignedMemoryAccesses(EVT VT, bool *Fast) const
-    LLVM_OVERRIDE;
+  virtual bool allowsUnalignedMemoryAccesses(EVT VT, unsigned AS,
+                                             bool *Fast) const LLVM_OVERRIDE;
   virtual bool isTruncateFree(Type *, Type *) const LLVM_OVERRIDE;
   virtual bool isTruncateFree(EVT, EVT) const LLVM_OVERRIDE;
   virtual const char *getTargetNodeName(unsigned Opcode) const LLVM_OVERRIDE;

lib/Target/SystemZ/SystemZISelLowering.cpp

 }
 
 bool SystemZTargetLowering::allowsUnalignedMemoryAccesses(EVT VT,
+                                                          unsigned,
                                                           bool *Fast) const {
   // Unaligned accesses should never be slower than the expanded version.
   // We check specifically for aligned accesses in the few cases where

lib/Target/X86/X86ISelLowering.h

     /// allowsUnalignedMemoryAccesses - Returns true if the target allows
     /// unaligned memory accesses. of the specified type. Returns whether it
     /// is "fast" by reference in the second argument.
-    virtual bool allowsUnalignedMemoryAccesses(EVT VT, bool *Fast) const;
+    virtual bool allowsUnalignedMemoryAccesses(EVT VT, unsigned AS,
+                                               bool *Fast) const;
 
     /// LowerOperation - Provide custom lowering hooks for some operations.
     ///

lib/Target/X86/X86ISelLowering.cpp

 }
 
 bool
-X86TargetLowering::allowsUnalignedMemoryAccesses(EVT VT, bool *Fast) const {
+X86TargetLowering::allowsUnalignedMemoryAccesses(EVT VT,
+                                                 unsigned,
+                                                 bool *Fast) const {
   if (Fast)
     *Fast = Subtarget->isUnalignedMemAccessFast();
   return true;