[x86] NFC: More refactoring to pave the way to extending this ISel logic to handle other x86 pseudos that carry flags and thus can't be matched by our ISel patterns with fused memory accesses.

[chandlerc]

Summary

Let me know if this stops making sense and I should just fold my actual
functional changes into this.

Depends on D37045.

[craig.topper]

Is your first version going to handle just registers or immediates too/

lib/Target/X86/X86ISelDAGToDAG.cpp

1959 ↗

(On Diff #112486)

Any real reason to pass the load sizes with this? Could we just define an order and just map the types to an index without a loop. Feels like the types starting getting repetitive if we extend this to ADD/SUB/AND/OR/XOR/ADC/SBB which all have this problem.

[chandlerc]

In D37088#850990, @craig.topper wrote:

Is your first version going to handle just registers or immediates too/

Not sure, haven't gotten there. Just carving off cleanups where I can.

lib/Target/X86/X86ISelDAGToDAG.cpp

1959 ↗

(On Diff #112486)

Good question. I went back and forth. My concern about just defining the order is getting it wrong. But honestly, I think that's not such an issue. I'll go back to hard coding the 4 integer types. We can make something fancier when we actually need it if that day ever arrives.

[chandlerc]

Update based on review.

[craig.topper]

LGTM with the one comment.

lib/Target/X86/X86ISelDAGToDAG.cpp

1945 ↗

(On Diff #112497)

Pass EVT by value.

[chandlerc]

Made change and landing, thanks!

llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp

for (SDNode::use_iterator FlagUI = UI->use_begin(),
       // Anything else: assume conservatively.
       default: return false;
       }
     }
   }
   return true;
 }
 
+/// Get the appropriate X86 opcode for an in-memory arithmetic operation that
+/// also sets flags.
+///
+/// FIXME: This is essentially re-implemneting a subset of the patterns for
+/// these instructions. Instead, we should compute this from the patterns
+/// somehow.
+///
+/// FIXME: Currently we only support integer operations.
+///
+/// If there is no X86 opcode, returns none.
+static Optional<unsigned> getFusedLdStWithFlagsOpcode(EVT LdVT, unsigned Opc) {
+  auto SelectSize = [&](unsigned Opc64, unsigned Opc32, unsigned Opc16,
+                        unsigned Opc8) -> Optional<unsigned> {
+    switch (LdVT.getSimpleVT().SimpleTy) {
+    case MVT::i64:
+      return Opc64;
+    case MVT::i32:
+      return Opc32;
+    case MVT::i16:
+      return Opc16;
+    case MVT::i8:
+      return Opc8;
+    default:
+      return None;
+    }
+  };
+  switch (Opc) {
+  default:
+    return None;
+  case X86ISD::DEC:
+    return SelectSize(X86::DEC64m, X86::DEC32m, X86::DEC16m, X86::DEC8m);
+  case X86ISD::INC:
+    return SelectSize(X86::INC64m, X86::INC32m, X86::INC16m, X86::INC8m);
+  }
+}
+
 /// Check whether or not the chain ending in StoreNode is suitable for doing
-/// the {load; increment or decrement; store} to modify transformation.
-static bool isLoadIncOrDecStore(StoreSDNode *StoreNode, unsigned Opc,
+/// the {load; op; store} to modify transformation.
+static bool isFusableLoadOpStorePattern(StoreSDNode *StoreNode,
                                         SDValue StoredVal, SelectionDAG *CurDAG,
-                                LoadSDNode* &LoadNode, SDValue &InputChain) {
-
-  // is the value stored the result of a DEC or INC?
-  if (!(Opc == X86ISD::DEC || Opc == X86ISD::INC)) return false;
-
+                                        LoadSDNode *&LoadNode,
+                                        SDValue &InputChain) {
   // is the stored value result 0 of the load?
   if (StoredVal.getResNo() != 0) return false;
 
   // are there other uses of the loaded value than the inc or dec?
   if (!StoredVal.getNode()->hasNUsesOfValue(1, 0)) return false;
 
   // is the store non-extending and non-indexed?
   if (!ISD::isNormalStore(StoreNode) || StoreNode->isNonTemporal())
     return false;
 
   SDValue Load = StoredVal->getOperand(0);
   // Is the stored value a non-extending and non-indexed load?
   if (!ISD::isNormalLoad(Load.getNode())) return false;
 
   // Return LoadNode by reference.
   LoadNode = cast<LoadSDNode>(Load);
-  // is the size of the value one that we can handle? (i.e. 64, 32, 16, or 8)
-  EVT LdVT = LoadNode->getMemoryVT();
-  if (LdVT != MVT::i64 && LdVT != MVT::i32 && LdVT != MVT::i16 &&
-      LdVT != MVT::i8)
-    return false;
 
   // Is store the only read of the loaded value?
   if (!Load.hasOneUse())
     return false;
 
   // Is the address of the store the same as the load?
   if (LoadNode->getBasePtr() != StoreNode->getBasePtr() ||
       LoadNode->getOffset() != StoreNode->getOffset())
if (ChainCheck)
                                    MVT::Other, ChainOps);
   }
   if (!ChainCheck)
     return false;
 
   return true;
 }
 
-/// Get the appropriate X86 opcode for an in-memory increment or decrement.
-/// Opc should be X86ISD::DEC or X86ISD::INC.
-static unsigned getFusedLdStOpcode(EVT &LdVT, unsigned Opc) {
-  if (Opc == X86ISD::DEC) {
-    if (LdVT == MVT::i64) return X86::DEC64m;
-    if (LdVT == MVT::i32) return X86::DEC32m;
-    if (LdVT == MVT::i16) return X86::DEC16m;
-    if (LdVT == MVT::i8)  return X86::DEC8m;
-  } else {
-    assert(Opc == X86ISD::INC && "unrecognized opcode");
-    if (LdVT == MVT::i64) return X86::INC64m;
-    if (LdVT == MVT::i32) return X86::INC32m;
-    if (LdVT == MVT::i16) return X86::INC16m;
-    if (LdVT == MVT::i8)  return X86::INC8m;
-  }
-  llvm_unreachable("unrecognized size for LdVT");
-}
-
 // Change a chain of {load; incr or dec; store} of the same value into
 // a simple increment or decrement through memory of that value, if the
 // uses of the modified value and its address are suitable.
 // The DEC64m tablegen pattern is currently not able to match the case where
 // the EFLAGS on the original DEC are used. (This also applies to
 // {INC,DEC}X{64,32,16,8}.)
 // We'll need to improve tablegen to allow flags to be transferred from a
 // node in the pattern to the result node.  probably with a new keyword
 // for example, we have this
 // def DEC64m : RI<0xFF, MRM1m, (outs), (ins i64mem:$dst), "dec{q}\t$dst",
 //  [(store (add (loadi64 addr:$dst), -1), addr:$dst),
 //   (implicit EFLAGS)]>;
 // but maybe need something like this
 // def DEC64m : RI<0xFF, MRM1m, (outs), (ins i64mem:$dst), "dec{q}\t$dst",
 //  [(store (add (loadi64 addr:$dst), -1), addr:$dst),
 //   (transferrable EFLAGS)]>;
 //
 // FIXME: This should handle a wide range of operations which support RMW
 // memory operands, not just inc and dec.
 bool X86DAGToDAGISel::foldLoadStoreIntoMemOperand(SDNode *Node) {
   StoreSDNode *StoreNode = cast<StoreSDNode>(Node);
   SDValue StoredVal = StoreNode->getOperand(1);
   unsigned Opc = StoredVal->getOpcode();
 
+  EVT MemVT = StoreNode->getMemoryVT();
+  if (!MemVT.isSimple())
+    return false;
+  Optional<unsigned> NewOpc = getFusedLdStWithFlagsOpcode(MemVT, Opc);
+  if (!NewOpc)
+    return false;
+
   LoadSDNode *LoadNode = nullptr;
   SDValue InputChain;
-  if (!isLoadIncOrDecStore(StoreNode, Opc, StoredVal, CurDAG, LoadNode,
+  if (!isFusableLoadOpStorePattern(StoreNode, StoredVal, CurDAG, LoadNode,
                                    InputChain))
     return false;
 
   SDValue Base, Scale, Index, Disp, Segment;
   if (!selectAddr(LoadNode, LoadNode->getBasePtr(), Base, Scale, Index, Disp,
                   Segment))
     return false;
 
   MachineSDNode::mmo_iterator MemOp = MF->allocateMemRefsArray(2);
   MemOp[0] = StoreNode->getMemOperand();
   MemOp[1] = LoadNode->getMemOperand();
   const SDValue Ops[] = {Base, Scale, Index, Disp, Segment, InputChain};
-  EVT LdVT = LoadNode->getMemoryVT();
-  unsigned newOpc = getFusedLdStOpcode(LdVT, Opc);
   MachineSDNode *Result =
-      CurDAG->getMachineNode(newOpc, SDLoc(Node), MVT::i32, MVT::Other, Ops);
+      CurDAG->getMachineNode(*NewOpc, SDLoc(Node), MVT::i32, MVT::Other, Ops);
   Result->setMemRefs(MemOp, MemOp + 2);
 
   ReplaceUses(SDValue(StoreNode, 0), SDValue(Result, 1));
   ReplaceUses(SDValue(StoredVal.getNode(), 1), SDValue(Result, 0));
   CurDAG->RemoveDeadNode(Node);
   return true;
 }