[TargetLowering] Make allowsMemoryAccess methode virtual.

[ThomasRaoux]

Summary

Rename old function to explicitly show that it cares only about alignment. The new allowsMemoryAccess call the function related to alignment by default and can be overridden by target to inform whether the memory access is legal or not.

[arsenm]

This is already a wrapper around the overridable allowsMisalignedMemoryAccesses

[craig.topper]

Aren't the targets intended to override allowsMisalignedMemoryAccesses? Does that not work for your usage?

[jmolloy]

Aren't the targets intended to override allowsMisalignedMemoryAccesses? Does that not work for your usage?

Unfortunately not. The target in question does not have byte or word stores. The intent is for the target hook implementation to return false based upon VT (a trunc-store doesn't change the alignment of the underlying pointer). I don't myself see a rationale why this function shouldn't be overridable, given that it isn't a no-op wrapper around allowsMisalignedMemoryAccesses.

[jmolloy]

(for more context, this is in the context of https://reviews.llvm.org/rL370576 . It's entirely possible I picked a suboptimal target hook, but allowsMemoryAccess seemed correct to me.).

[arsenm]

It seems to me like this is a workaround for the quite inadequate system for defining legality for a load/store. I think allowsMemoryAccess is poorly named; It exists to just check if the type is aligned before calling allowsMisalignedMemoryAccesses. Your problem sounds more like we need a better system for checking if a store/trunc store is legal

[ThomasRaoux]

In D67121#1656161, @arsenm wrote:

It seems to me like this is a workaround for the quite inadequate system for defining legality for a load/store. I think allowsMemoryAccess is poorly named; It exists to just check if the type is aligned before calling allowsMisalignedMemoryAccesses. Your problem sounds more like we need a better system for checking if a store/trunc store is legal

The comment mentions "Return true if the target supports a memory access of this type for the given address space and alignment". So I assumed it is responsible for more than checking alignment. Is this not correct?

[jmolloy]

Your problem sounds more like we need a better system for checking if a store/trunc store is legal

I do sort of agree but this is potentially a deep hole to dive into. In particular the issue here isn't actually trunc-stores, as a trunc-store is equivalent to trunc + AssertZext + store. It's that we're creating a store with a memory bitwidth that isn't supported, and undoing that is really hard.

I'm not sure I completely understand your objection though - You've mentioned how allowsMemoryAccess is intended to be used, but why is it intended in that way? I think I may lack some backcontext.

The comment, signature and name appear to me to be the exact test needed in this case?

[arsenm]

My concern is about growing multiple, parallel concepts related to load/store legality and the existing mess of hooks to control these sort of combines and would like to avoid potentially making it worse.

We already have a few too many hooks for controlling this sort of thing, and none them seem really sufficient or well thought out. We have the basic register type legality check, whether the load/store (for a type only) is legal, checks for whether truncstore/extload are legal, and allowsMisalignedMemoryAccesses. Beyond that, we have hooks like isLoadBitCastBeneficial/isStoreBitCastBeneficial, canMergeStoresTo, mergeStoresAfterLegalization, and possibly a few more.

Things would probably be better if we had some variant of allowsMemoryAccess that would totally subsume the existing set of legality checks. On the other hand, I don't really want to spend much time thinking about how to make SelectionDAG better at this point.

[jmolloy]

On the other hand, I don't really want to spend much time thinking about how to make SelectionDAG better at this point.

I agree, that's kind of my worry with opening this pandora's box. Changing hooks in SDAG and maintaining performance parity is very hard, and we have a longer term solution in Global Isel.

Given that, and given the NFC-ness of this patch, are you happy for it to go in?

[arsenm]

In D67121#1656980, @jmolloy wrote:

On the other hand, I don't really want to spend much time thinking about how to make SelectionDAG better at this point.

I agree, that's kind of my worry with opening this pandora's box. Changing hooks in SDAG and maintaining performance parity is very hard, and we have a longer term solution in Global Isel.

Given that, and given the NFC-ness of this patch, are you happy for it to go in?

I'm not happy about it but not too opposed. I'd like someone else's opinion

[arsenm]

In D67121#1663397, @arsenm wrote:

In D67121#1656980, @jmolloy wrote:

On the other hand, I don't really want to spend much time thinking about how to make SelectionDAG better at this point.

I agree, that's kind of my worry with opening this pandora's box. Changing hooks in SDAG and maintaining performance parity is very hard, and we have a longer term solution in Global Isel.

Given that, and given the NFC-ness of this patch, are you happy for it to go in?

I'm not happy about it but not too opposed. I'd like someone else's opinion

I would somewhat prefer we split the current function into something like memoryAccessIsLegalForAlignment that has the current behavior, and this would be a separate function defaulting to calling it (Yes, I know this makes the problem worse)

[ThomasRaoux]

I added a new function allowsMemoryAccessForAlignment to handle the behavior of the previous allowsMemoryAccess function. I replaced the calls every except for DAGCombine as my understanding is that this place really mean to check that the memory access is legal and won't require legalization to expensive code sequence.

Is that what you had in mind?

[ThomasRaoux]

In D67121#1663409, @arsenm wrote:

In D67121#1663397, @arsenm wrote:

In D67121#1656980, @jmolloy wrote:

On the other hand, I don't really want to spend much time thinking about how to make SelectionDAG better at this point.

I agree, that's kind of my worry with opening this pandora's box. Changing hooks in SDAG and maintaining performance parity is very hard, and we have a longer term solution in Global Isel.

Given that, and given the NFC-ness of this patch, are you happy for it to go in?

I'm not happy about it but not too opposed. I'd like someone else's opinion

I would somewhat prefer we split the current function into something like memoryAccessIsLegalForAlignment that has the current behavior, and this would be a separate function defaulting to calling it (Yes, I know this makes the problem worse)

@arsenm , are you okay with this set of changes?

[arsenm]

LGTM

llvm/trunk/include/llvm/CodeGen/TargetLowering.h

   /// LLT handling variant.
   virtual bool allowsMisalignedMemoryAccesses(
       LLT, unsigned AddrSpace = 0, unsigned Align = 1,
       MachineMemOperand::Flags Flags = MachineMemOperand::MONone,
       bool * /*Fast*/ = nullptr) const {
     return false;
   }
 
+  /// This function returns true if the memory access is aligned or if the
+  /// target allows this specific unaligned memory access. If the access is
+  /// allowed, the optional final parameter returns if the access is also fast
+  /// (as defined by the target).
+  bool allowsMemoryAccessForAlignment(
+      LLVMContext &Context, const DataLayout &DL, EVT VT,
+      unsigned AddrSpace = 0, unsigned Alignment = 1,
+      MachineMemOperand::Flags Flags = MachineMemOperand::MONone,
+      bool *Fast = nullptr) const;
+
+  /// Return true if the memory access of this type is aligned or if the target
+  /// allows this specific unaligned access for the given MachineMemOperand.
+  /// If the access is allowed, the optional final parameter returns if the
+  /// access is also fast (as defined by the target).
+  bool allowsMemoryAccessForAlignment(LLVMContext &Context,
+                                      const DataLayout &DL, EVT VT,
+                                      const MachineMemOperand &MMO,
+                                      bool *Fast = nullptr) const;
+
   /// Return true if the target supports a memory access of this type for the
   /// given address space and alignment. If the access is allowed, the optional
   /// final parameter returns if the access is also fast (as defined by the
   /// target).
-  bool
+  virtual bool
   allowsMemoryAccess(LLVMContext &Context, const DataLayout &DL, EVT VT,
                      unsigned AddrSpace = 0, unsigned Alignment = 1,
                      MachineMemOperand::Flags Flags = MachineMemOperand::MONone,
                      bool *Fast = nullptr) const;
 
   /// Return true if the target supports a memory access of this type for the
   /// given MachineMemOperand. If the access is allowed, the optional
   /// final parameter returns if the access is also fast (as defined by the

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

bool DAGCombiner::isLegalNarrowLdSt(LSBaseSDNode *LDST,
   // Don't change the width of a volatile or atomic loads.
   if (!LDST->isSimple())
     return false;
 
   // Verify that we are actually reducing a load width here.
   if (LDST->getMemoryVT().getSizeInBits() < MemVT.getSizeInBits())
     return false;
 
-  // Ensure that this isn't going to produce an unsupported unaligned access.
+  // Ensure that this isn't going to produce an unsupported memory access.
   if (ShAmt &&
       !TLI.allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), MemVT,
                               LDST->getAddressSpace(), ShAmt / 8,
                               LDST->getMemOperand()->getFlags()))
     return false;
 
   // It's not possible to generate a constant of extended or untyped type.
   EVT PtrType = LDST->getBasePtr().getValueType();

llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

if (!ST->isTruncatingStore()) {
     MVT VT = Value.getSimpleValueType();
     switch (TLI.getOperationAction(ISD::STORE, VT)) {
     default: llvm_unreachable("This action is not supported yet!");
     case TargetLowering::Legal: {
       // If this is an unaligned store and the target doesn't support it,
       // expand it.
       EVT MemVT = ST->getMemoryVT();
       const DataLayout &DL = DAG.getDataLayout();
-      if (!TLI.allowsMemoryAccess(*DAG.getContext(), DL, MemVT,
+      if (!TLI.allowsMemoryAccessForAlignment(*DAG.getContext(), DL, MemVT,
                                               *ST->getMemOperand())) {
         LLVM_DEBUG(dbgs() << "Expanding unsupported unaligned store\n");
         SDValue Result = TLI.expandUnalignedStore(ST, DAG);
         ReplaceNode(SDValue(ST, 0), Result);
       } else
         LLVM_DEBUG(dbgs() << "Legal store\n");
       break;
     }
     case TargetLowering::Custom: {
if (StWidth != StVT.getStoreSizeInBits()) {
     ReplaceNode(SDValue(Node, 0), Result);
   } else {
     switch (TLI.getTruncStoreAction(ST->getValue().getValueType(), StVT)) {
     default: llvm_unreachable("This action is not supported yet!");
     case TargetLowering::Legal: {
       EVT MemVT = ST->getMemoryVT();
       // If this is an unaligned store and the target doesn't support it,
       // expand it.
-      if (!TLI.allowsMemoryAccess(*DAG.getContext(), DL, MemVT,
+      if (!TLI.allowsMemoryAccessForAlignment(*DAG.getContext(), DL, MemVT,
                                               *ST->getMemOperand())) {
         SDValue Result = TLI.expandUnalignedStore(ST, DAG);
         ReplaceNode(SDValue(ST, 0), Result);
       }
       break;
     }
     case TargetLowering::Custom: {
       SDValue Res = TLI.LowerOperation(SDValue(Node, 0), DAG);
       if (Res && Res != SDValue(Node, 0))
if (ExtType == ISD::NON_EXTLOAD) {
 
     switch (TLI.getOperationAction(Node->getOpcode(), VT)) {
     default: llvm_unreachable("This action is not supported yet!");
     case TargetLowering::Legal: {
       EVT MemVT = LD->getMemoryVT();
       const DataLayout &DL = DAG.getDataLayout();
       // If this is an unaligned load and the target doesn't support it,
       // expand it.
-      if (!TLI.allowsMemoryAccess(*DAG.getContext(), DL, MemVT,
+      if (!TLI.allowsMemoryAccessForAlignment(*DAG.getContext(), DL, MemVT,
                                               *LD->getMemOperand())) {
         std::tie(RVal, RChain) = TLI.expandUnalignedLoad(LD, DAG);
       }
       break;
     }
     case TargetLowering::Custom:
       if (SDValue Res = TLI.LowerOperation(RVal, DAG)) {
         RVal = Res;
         RChain = Res.getValue(1);

llvm/trunk/lib/CodeGen/TargetLoweringBase.cpp

 /// getByValTypeAlignment - Return the desired alignment for ByVal aggregate
 /// function arguments in the caller parameter area.  This is the actual
 /// alignment, not its logarithm.
 unsigned TargetLoweringBase::getByValTypeAlignment(Type *Ty,
                                                    const DataLayout &DL) const {
   return DL.getABITypeAlignment(Ty);
 }
 
-bool TargetLoweringBase::allowsMemoryAccess(LLVMContext &Context,
-                                            const DataLayout &DL, EVT VT,
-                                            unsigned AddrSpace,
-                                            unsigned Alignment,
-                                            MachineMemOperand::Flags Flags,
-                                            bool *Fast) const {
+bool TargetLoweringBase::allowsMemoryAccessForAlignment(
+    LLVMContext &Context, const DataLayout &DL, EVT VT, unsigned AddrSpace,
+    unsigned Alignment, MachineMemOperand::Flags Flags, bool *Fast) const {
   // Check if the specified alignment is sufficient based on the data layout.
   // TODO: While using the data layout works in practice, a better solution
   // would be to implement this check directly (make this a virtual function).
   // For example, the ABI alignment may change based on software platform while
   // this function should only be affected by hardware implementation.
   Type *Ty = VT.getTypeForEVT(Context);
   if (Alignment >= DL.getABITypeAlignment(Ty)) {
     // Assume that an access that meets the ABI-specified alignment is fast.
     if (Fast != nullptr)
       *Fast = true;
     return true;
   }
 
   // This is a misaligned access.
   return allowsMisalignedMemoryAccesses(VT, AddrSpace, Alignment, Flags, Fast);
 }
 
+bool TargetLoweringBase::allowsMemoryAccessForAlignment(
+    LLVMContext &Context, const DataLayout &DL, EVT VT,
+    const MachineMemOperand &MMO, bool *Fast) const {
+  return allowsMemoryAccessForAlignment(Context, DL, VT, MMO.getAddrSpace(),
+                                        MMO.getAlignment(), MMO.getFlags(),
+                                        Fast);
+}
+
+bool TargetLoweringBase::allowsMemoryAccess(
+    LLVMContext &Context, const DataLayout &DL, EVT VT, unsigned AddrSpace,
+    unsigned Alignment, MachineMemOperand::Flags Flags, bool *Fast) const {
+  return allowsMemoryAccessForAlignment(Context, DL, VT, AddrSpace, Alignment,
+                                        Flags, Fast);
+}
+
 bool TargetLoweringBase::allowsMemoryAccess(LLVMContext &Context,
                                             const DataLayout &DL, EVT VT,
                                             const MachineMemOperand &MMO,
                                             bool *Fast) const {
   return allowsMemoryAccess(Context, DL, VT, MMO.getAddrSpace(),
                             MMO.getAlignment(), MMO.getFlags(), Fast);
 }
 

llvm/trunk/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

bool AMDGPUTargetLowering::isLoadBitCastBeneficial(EVT LoadTy, EVT CastTy,
 
   unsigned LScalarSize = LoadTy.getScalarSizeInBits();
   unsigned CastScalarSize = CastTy.getScalarSizeInBits();
 
   if ((LScalarSize >= CastScalarSize) && (CastScalarSize < 32))
     return false;
 
   bool Fast = false;
-  return allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), CastTy,
-                            MMO, &Fast) && Fast;
+  return allowsMemoryAccessForAlignment(*DAG.getContext(), DAG.getDataLayout(),
+                                        CastTy, MMO, &Fast) &&
+         Fast;
 }
 
 // SI+ has instructions for cttz / ctlz for 32-bit values. This is probably also
 // profitable with the expansion for 64-bit since it's generally good to
 // speculate things.
 // FIXME: These should really have the size as a parameter.
 bool AMDGPUTargetLowering::isCheapToSpeculateCttz() const {
   return true;

llvm/trunk/lib/Target/AMDGPU/SIISelLowering.cpp

SDValue SITargetLowering::LowerLOAD(SDValue Op, SelectionDAG &DAG) const {
   }
 
   if (!MemVT.isVector())
     return SDValue();
 
   assert(Op.getValueType().getVectorElementType() == MVT::i32 &&
          "Custom lowering for non-i32 vectors hasn't been implemented.");
 
-  if (!allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), MemVT,
-                          *Load->getMemOperand())) {
+  if (!allowsMemoryAccessForAlignment(*DAG.getContext(), DAG.getDataLayout(),
+                                      MemVT, *Load->getMemOperand())) {
     SDValue Ops[2];
     std::tie(Ops[0], Ops[1]) = expandUnalignedLoad(Load, DAG);
     return DAG.getMergeValues(Ops, DL);
   }
 
   unsigned Alignment = Load->getAlignment();
   unsigned AS = Load->getAddressSpace();
   if (Subtarget->hasLDSMisalignedBug() &&
if (VT == MVT::i1) {
     return DAG.getTruncStore(Store->getChain(), DL,
        DAG.getSExtOrTrunc(Store->getValue(), DL, MVT::i32),
        Store->getBasePtr(), MVT::i1, Store->getMemOperand());
   }
 
   assert(VT.isVector() &&
          Store->getValue().getValueType().getScalarType() == MVT::i32);
 
-  if (!allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), VT,
-                          *Store->getMemOperand())) {
+  if (!allowsMemoryAccessForAlignment(*DAG.getContext(), DAG.getDataLayout(),
+                                      VT, *Store->getMemOperand())) {
     return expandUnalignedStore(Store, DAG);
   }
 
   unsigned AS = Store->getAddressSpace();
   if (Subtarget->hasLDSMisalignedBug() &&
       AS == AMDGPUAS::FLAT_ADDRESS &&
       Store->getAlignment() < VT.getStoreSize() && VT.getSizeInBits() > 32) {
     return SplitVectorStore(Op, DAG);

llvm/trunk/lib/Target/Hexagon/HexagonISelLowering.cpp

HexagonTargetLowering::LowerUnalignedLoad(SDValue Op, SelectionDAG &DAG)
   // If the load aligning is disabled or the load can be broken up into two
   // smaller legal loads, do the default (target-independent) expansion.
   bool DoDefault = false;
   // Handle it in the default way if this is an indexed load.
   if (!LN->isUnindexed())
     DoDefault = true;
 
   if (!AlignLoads) {
-    if (allowsMemoryAccess(Ctx, DL, LN->getMemoryVT(), *LN->getMemOperand()))
+    if (allowsMemoryAccessForAlignment(Ctx, DL, LN->getMemoryVT(),
+                                       *LN->getMemOperand()))
       return Op;
     DoDefault = true;
   }
   if (!DoDefault && (2 * HaveAlign) == NeedAlign) {
     // The PartTy is the equivalent of "getLoadableTypeOfSize(HaveAlign)".
     MVT PartTy = HaveAlign <= 8 ? MVT::getIntegerVT(8 * HaveAlign)
                                 : MVT::getVectorVT(MVT::i8, HaveAlign);
-    DoDefault = allowsMemoryAccess(Ctx, DL, PartTy, *LN->getMemOperand());
+    DoDefault =
+        allowsMemoryAccessForAlignment(Ctx, DL, PartTy, *LN->getMemOperand());
   }
   if (DoDefault) {
     std::pair<SDValue, SDValue> P = expandUnalignedLoad(LN, DAG);
     return DAG.getMergeValues({P.first, P.second}, dl);
   }
 
   // The code below generates two loads, both aligned as NeedAlign, and
   // with the distance of NeedAlign between them. For that to cover the

llvm/trunk/lib/Target/NVPTX/NVPTXISelLowering.cpp

SDValue NVPTXTargetLowering::LowerLOAD(SDValue Op, SelectionDAG &DAG) const {
   if (Op.getValueType() == MVT::i1)
     return LowerLOADi1(Op, DAG);
 
   // v2f16 is legal, so we can't rely on legalizer to handle unaligned
   // loads and have to handle it here.
   if (Op.getValueType() == MVT::v2f16) {
     LoadSDNode *Load = cast<LoadSDNode>(Op);
     EVT MemVT = Load->getMemoryVT();
-    if (!allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), MemVT,
-                            *Load->getMemOperand())) {
+    if (!allowsMemoryAccessForAlignment(*DAG.getContext(), DAG.getDataLayout(),
+                                        MemVT, *Load->getMemOperand())) {
       SDValue Ops[2];
       std::tie(Ops[0], Ops[1]) = expandUnalignedLoad(Load, DAG);
       return DAG.getMergeValues(Ops, SDLoc(Op));
     }
   }
 
   return SDValue();
 }
SDValue NVPTXTargetLowering::LowerSTORE(SDValue Op, SelectionDAG &DAG) const {
   EVT VT = Store->getMemoryVT();
 
   if (VT == MVT::i1)
     return LowerSTOREi1(Op, DAG);
 
   // v2f16 is legal, so we can't rely on legalizer to handle unaligned
   // stores and have to handle it here.
   if (VT == MVT::v2f16 &&
-      !allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), VT,
-                          *Store->getMemOperand()))
+      !allowsMemoryAccessForAlignment(*DAG.getContext(), DAG.getDataLayout(),
+                                      VT, *Store->getMemOperand()))
     return expandUnalignedStore(Store, DAG);
 
   if (VT.isVector())
     return LowerSTOREVector(Op, DAG);
 
   return SDValue();
 }
 

llvm/trunk/lib/Target/XCore/XCoreISelLowering.cpp

 SDValue XCoreTargetLowering::LowerLOAD(SDValue Op, SelectionDAG &DAG) const {
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   LLVMContext &Context = *DAG.getContext();
   LoadSDNode *LD = cast<LoadSDNode>(Op);
   assert(LD->getExtensionType() == ISD::NON_EXTLOAD &&
          "Unexpected extension type");
   assert(LD->getMemoryVT() == MVT::i32 && "Unexpected load EVT");
 
-  if (allowsMemoryAccess(Context, DAG.getDataLayout(), LD->getMemoryVT(),
-                         *LD->getMemOperand()))
+  if (allowsMemoryAccessForAlignment(Context, DAG.getDataLayout(),
+                                     LD->getMemoryVT(), *LD->getMemOperand()))
     return SDValue();
 
   SDValue Chain = LD->getChain();
   SDValue BasePtr = LD->getBasePtr();
   SDLoc DL(Op);
 
   if (!LD->isVolatile()) {
     const GlobalValue *GV;

 }
 
 SDValue XCoreTargetLowering::LowerSTORE(SDValue Op, SelectionDAG &DAG) const {
   LLVMContext &Context = *DAG.getContext();
   StoreSDNode *ST = cast<StoreSDNode>(Op);
   assert(!ST->isTruncatingStore() && "Unexpected store type");
   assert(ST->getMemoryVT() == MVT::i32 && "Unexpected store EVT");
 
-  if (allowsMemoryAccess(Context, DAG.getDataLayout(), ST->getMemoryVT(),
-                         *ST->getMemOperand()))
+  if (allowsMemoryAccessForAlignment(Context, DAG.getDataLayout(),
+                                     ST->getMemoryVT(), *ST->getMemOperand()))
     return SDValue();
 
   SDValue Chain = ST->getChain();
   SDValue BasePtr = ST->getBasePtr();
   SDValue Value = ST->getValue();
   SDLoc dl(Op);
 
   if (ST->getAlignment() == 2) {
if (N->getValueType(0) == MVT::i64 &&
       return DAG.getNode(ISD::BUILD_PAIR, dl, MVT::i64, Lo, Hi);
     }
   }
   break;
   case ISD::STORE: {
     // Replace unaligned store of unaligned load with memmove.
     StoreSDNode *ST = cast<StoreSDNode>(N);
     if (!DCI.isBeforeLegalize() ||
-        allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(),
-                           ST->getMemoryVT(), *ST->getMemOperand()) ||
+        allowsMemoryAccessForAlignment(*DAG.getContext(), DAG.getDataLayout(),
+                                       ST->getMemoryVT(),
+                                       *ST->getMemOperand()) ||
         ST->isVolatile() || ST->isIndexed()) {
       break;
     }
     SDValue Chain = ST->getChain();
 
     unsigned StoreBits = ST->getMemoryVT().getStoreSizeInBits();
     assert((StoreBits % 8) == 0 &&
            "Store size in bits must be a multiple of 8");