[DAGCombine] Refactor ReduceLoadWidth

[samparker]

Summary

visitAND attempts to narrow the width of extending loads that are then masked off. ReduceLoadWidth already exists for a similar purpose and handles shifts, so I've moved the code to handle AND nodes there.

[fhahn]

Removing the unneeded LoadedVT seems like a straight-forward NFC and I think it would be worth to submit it for review as an isolated change, as it does not seem related to moving some code to ReduceLoadWidth.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

7969

If we support extension using AND, I think we should also update the comment here.

7999

I am probably missing something, but I am not sure if I see the real benefit of moving this code here. It seems the visitAND was the right place to do it, without requiring additional checks. However, it seems like there is potential to simplify the code in visitAND

[samparker]

Hi Florian,

Thanks for the comments, I will revert the LoadedVT change.

cheers,
sam

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

7999

Both visitAND and ReduceLoadWidth contained essentially the same code to generate the new load, so it seems reasonable to me to contain this logic within the sensibly named function. In D39604 I also wish to directly use this function to combine ANDs and hopefully it can more helpful when anyone else wants to do similar.

[samparker]

Removed the unrelated API change and updated the description of ReduceLoadWidth.

[samparker]

Hi Florian,

Do you have any other suggestions?

cheers

[fhahn]

Looks like a reasonable refactor to me helping with D39604. Adding Simon & Eli as reviewers as I think they know the area better then myself.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

7999

Ah yes, there is some shared code to generated the new load, I missed initially! ReduceLoadWidth also contains a few more legality checks.

[efriedma]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8026

Could this block of code be simplified? As long as you set ExtType and ExtVT correctly, I think the rest of the checks already exist in some form, by analogy to the transforms for ISD::SIGN_EXTEND_IN_REG.

[samparker]

Removed most of the checks performed on the load, leaving just the SEXT check.

[samparker]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8026

Thanks Eli, indeed most of the checks that performed again.

[fhahn]

Thanks Sam, looks good to me. Please wait a day or two with committing, to give others a chance to raise additional concerns.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8034

IIRC your first version of the patch removed to unneeded LoadedVT argument. I think this would be a worthwhile simplification to get in as NFC independently.

[RKSimon]

Very minor comment

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8031

use bool here - we typically only use auto for casts

[samparker]

@fhahn @RKSimon @eli.friedman I made some small changes before committing, could someone please check them out? My development branch only had x86, Arm and Aarch64 targets built and so when I went to run my pre-commit tests, some were failing for AMDGPU and NVPTX. The issues were that even though some checks are duplicated, I have an early return in the new block which still requires those checks. Also I had missed how any_extend nodes are combined, so I re-added some code that was lost during the refactor.

[efriedma]

I'm still kind of concerned that this is messier than it needs to be. I guess I'll try experimenting a bit.

[efriedma]

See D40667.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

     /// of folding (mul (add x, c1), c2) -> (add (mul x, c2), c1*c2).
     /// MulNode is the original multiply, AddNode is (add x, c1),
     /// and ConstNode is c2.
     bool isMulAddWithConstProfitable(SDNode *MulNode,
                                      SDValue &AddNode,
                                      SDValue &ConstNode);
 
     /// This is a helper function for visitAND and visitZERO_EXTEND.  Returns
-    /// true if the (and (load x) c) pattern matches an extload.  ExtVT returns
-    /// the type of the loaded value to be extended.  LoadedVT returns the type
-    /// of the original loaded value.  NarrowLoad returns whether the load would
-    /// need to be narrowed in order to match.
+    /// true if the (and (load x) c) pattern matches an extload. ExtVT returns
+    /// the type of the loaded value to be extended. NarrowLoad returns whether
+    /// the load would need to be narrowed in order to match.
     bool isAndLoadExtLoad(ConstantSDNode *AndC, LoadSDNode *LoadN,
-                          EVT LoadResultTy, EVT &ExtVT, EVT &LoadedVT,
-                          bool &NarrowLoad);
+                          EVT LoadResultTy, EVT &ExtVT, bool &NarrowLoad);
 
     /// Helper function for MergeConsecutiveStores which merges the
     /// component store chains.
     SDValue getMergeStoreChains(SmallVectorImpl<MemOpLink> &StoreNodes,
                                 unsigned NumStores);
 
     /// This is a helper function for MergeConsecutiveStores. When the
     /// source elements of the consecutive stores are all constants or

       }
     }
   }
 
   return SDValue();
 }
 
 bool DAGCombiner::isAndLoadExtLoad(ConstantSDNode *AndC, LoadSDNode *LoadN,
-                                   EVT LoadResultTy, EVT &ExtVT, EVT &LoadedVT,
+                                   EVT LoadResultTy, EVT &ExtVT,
                                    bool &NarrowLoad) {
   if (!AndC->getAPIntValue().isMask())
     return false;
 
   unsigned ActiveBits = AndC->getAPIntValue().countTrailingOnes();
 
   ExtVT = EVT::getIntegerVT(*DAG.getContext(), ActiveBits);
-  LoadedVT = LoadN->getMemoryVT();
+  EVT LoadedVT = LoadN->getMemoryVT();
 
   if (ExtVT == LoadedVT &&
       (!LegalOperations ||
        TLI.isLoadExtLegal(ISD::ZEXTLOAD, LoadResultTy, ExtVT))) {
     // ZEXTLOAD will match without needing to change the size of the value being
     // loaded.
     NarrowLoad = false;
     return true;

   }
 
   // fold (and (load x), 255) -> (zextload x, i8)
   // fold (and (extload x, i16), 255) -> (zextload x, i8)
   // fold (and (any_ext (extload x, i16)), 255) -> (zextload x, i8)
   if (!VT.isVector() && N1C && (N0.getOpcode() == ISD::LOAD ||
                                 (N0.getOpcode() == ISD::ANY_EXTEND &&
                                  N0.getOperand(0).getOpcode() == ISD::LOAD))) {
-    bool HasAnyExt = N0.getOpcode() == ISD::ANY_EXTEND;
-    LoadSDNode *LN0 = HasAnyExt
-      ? cast<LoadSDNode>(N0.getOperand(0))
-      : cast<LoadSDNode>(N0);
-    if (LN0->getExtensionType() != ISD::SEXTLOAD &&
-        LN0->isUnindexed() && N0.hasOneUse() && SDValue(LN0, 0).hasOneUse()) {
-      auto NarrowLoad = false;
-      EVT LoadResultTy = HasAnyExt ? LN0->getValueType(0) : VT;
-      EVT ExtVT, LoadedVT;
-      if (isAndLoadExtLoad(N1C, LN0, LoadResultTy, ExtVT, LoadedVT,
-                           NarrowLoad)) {
-        if (!NarrowLoad) {
-          SDValue NewLoad =
-            DAG.getExtLoad(ISD::ZEXTLOAD, SDLoc(LN0), LoadResultTy,
-                           LN0->getChain(), LN0->getBasePtr(), ExtVT,
-                           LN0->getMemOperand());
-          AddToWorklist(N);
-          CombineTo(LN0, NewLoad, NewLoad.getValue(1));
-          return SDValue(N, 0);   // Return N so it doesn't get rechecked!
-        } else {
-          EVT PtrType = LN0->getOperand(1).getValueType();
-
-          unsigned Alignment = LN0->getAlignment();
-          SDValue NewPtr = LN0->getBasePtr();
-
-          // For big endian targets, we need to add an offset to the pointer
-          // to load the correct bytes.  For little endian systems, we merely
-          // need to read fewer bytes from the same pointer.
-          if (DAG.getDataLayout().isBigEndian()) {
-            unsigned LVTStoreBytes = LoadedVT.getStoreSize();
-            unsigned EVTStoreBytes = ExtVT.getStoreSize();
-            unsigned PtrOff = LVTStoreBytes - EVTStoreBytes;
-            SDLoc DL(LN0);
-            NewPtr = DAG.getNode(ISD::ADD, DL, PtrType,
-                                 NewPtr, DAG.getConstant(PtrOff, DL, PtrType));
-            Alignment = MinAlign(Alignment, PtrOff);
-          }
-
-          AddToWorklist(NewPtr.getNode());
-
-          SDValue Load = DAG.getExtLoad(
-              ISD::ZEXTLOAD, SDLoc(LN0), LoadResultTy, LN0->getChain(), NewPtr,
-              LN0->getPointerInfo(), ExtVT, Alignment,
-              LN0->getMemOperand()->getFlags(), LN0->getAAInfo());
-          AddToWorklist(N);
-          CombineTo(LN0, Load, Load.getValue(1));
-          return SDValue(N, 0);   // Return N so it doesn't get rechecked!
-        }
-      }
+    if (SDValue Res = ReduceLoadWidth(N)) {
+      AddToWorklist(N);
+      CombineTo(N0.getNode(), Res, Res.getValue(1));
+      return SDValue(N, 0);
     }
   }
 
   if (SDValue Combined = visitANDLike(N0, N1, N))
     return Combined;
 
   // Simplify: (and (op x...), (op y...))  -> (op (and x, y))
   if (N0.getOpcode() == N1.getOpcode())

     if (LN0->getExtensionType() != ISD::SEXTLOAD && LN0->isUnindexed()) {
       bool DoXform = true;
       SmallVector<SDNode*, 4> SetCCs;
       if (!N0.hasOneUse()) {
         if (N0.getOpcode() == ISD::AND) {
           auto *AndC = cast<ConstantSDNode>(N0.getOperand(1));
           auto NarrowLoad = false;
           EVT LoadResultTy = AndC->getValueType(0);
-          EVT ExtVT, LoadedVT;
-          if (isAndLoadExtLoad(AndC, LN0, LoadResultTy, ExtVT, LoadedVT,
-                               NarrowLoad))
+          EVT ExtVT;
+          if (isAndLoadExtLoad(AndC, LN0, LoadResultTy, ExtVT, NarrowLoad))
             DoXform = false;
         }
         if (DoXform)
           DoXform = ExtendUsesToFormExtLoad(N, N0.getOperand(0),
                                             ISD::ZERO_EXTEND, SetCCs, TLI);
       }
       if (DoXform) {
         SDValue ExtLoad = DAG.getExtLoad(ISD::ZEXTLOAD, SDLoc(LN0), VT,

     SDValue NewAssert = DAG.getNode(Opcode, DL, BigA.getValueType(),
                                     BigA.getOperand(0), MinAssertVTVal);
     return DAG.getNode(ISD::TRUNCATE, DL, N->getValueType(0), NewAssert);
   }
 
   return SDValue();
 }
 
 /// If the result of a wider load is shifted to right of N  bits and then
 /// truncated to a narrower type and where N is a multiple of number of bits of
 /// the narrower type, transform it to a narrower load from address + N / num of
 /// bits of new type. If the result is to be extended, also fold the extension
 /// to form a extending load.
 SDValue DAGCombiner::ReduceLoadWidth(SDNode *N) {
   unsigned Opc = N->getOpcode();
 
   ISD::LoadExtType ExtType = ISD::NON_EXTLOAD;

   } else if (Opc == ISD::SRL) {
     // Another special-case: SRL is basically zero-extending a narrower value.
     ExtType = ISD::ZEXTLOAD;
     N0 = SDValue(N, 0);
     ConstantSDNode *N01 = dyn_cast<ConstantSDNode>(N0.getOperand(1));
     if (!N01) return SDValue();
     ExtVT = EVT::getIntegerVT(*DAG.getContext(),
                               VT.getSizeInBits() - N01->getZExtValue());
+  } else if (Opc == ISD::AND) {
+    bool HasAnyExt = N0.getOpcode() == ISD::ANY_EXTEND;
+    LoadSDNode *LN0 =
+      HasAnyExt ? cast<LoadSDNode>(N0.getOperand(0)) : cast<LoadSDNode>(N0);
+
+    if (LN0->getExtensionType() == ISD::SEXTLOAD ||
+        !LN0->isUnindexed() || !N0.hasOneUse() || !SDValue(LN0, 0).hasOneUse())
+      return SDValue();
+
+    auto NarrowLoad = false;
+    ExtType = ISD::ZEXTLOAD;
+    VT = HasAnyExt ? LN0->getValueType(0) : VT;
+    auto N1C = dyn_cast<ConstantSDNode>(N->getOperand(1));
+    if (!N1C)
+      return SDValue();
+
+    if (!isAndLoadExtLoad(N1C, LN0, VT, ExtVT, NarrowLoad))
+      return SDValue();
+
+    if (!NarrowLoad)
+      return DAG.getExtLoad(ISD::ZEXTLOAD, SDLoc(LN0), VT,
+                            LN0->getChain(), LN0->getBasePtr(), ExtVT,
+                            LN0->getMemOperand());
   }
   if (LegalOperations && !TLI.isLoadExtLegal(ExtType, VT, ExtVT))
     return SDValue();
 
   unsigned EVTBits = ExtVT.getSizeInBits();
 
   // Do not generate loads of non-round integer types since these can
   // be expensive (and would be wrong if the type is not byte sized).
   if (!ExtVT.isRound())
     return SDValue();
 
   unsigned ShAmt = 0;
   if (N0.getOpcode() == ISD::SRL && N0.hasOneUse()) {
     if (ConstantSDNode *N01 = dyn_cast<ConstantSDNode>(N0.getOperand(1))) {
       ShAmt = N01->getZExtValue();
       // Is the shift amount a multiple of size of VT?
       if ((ShAmt & (EVTBits-1)) == 0) {
         N0 = N0.getOperand(0);
         // Is the load width a multiple of size of VT?
         if ((N0.getValueSizeInBits() & (EVTBits-1)) != 0)
           return SDValue();