DAG: Tolerate non-MemSDNOdes for OPC_RecordMemRef

[arsenm]

Summary

When intrinsics are allowed to have mem operands (D40978), there
are two ways this can happen. First is an intrinsic
that is marked has having a mem operand, but is not handled
by getTgtMemIntrinsic.

The second way can occur even for intrinsics which do not
have a mem operand. It seems the selector table does
some kind of sorting based on the opcode, and the
mem ref recording can happen in the same scope for
intrinsics that both do and do not have mem refs.
I haven't been able to figure out exactly why this happens
(although it happens even with the matcher optimizations disabled).
I'm not sure if it's worth trying to avoid hitting this for
these nodes since I think it's still reasonable to handle
this in case getTgtMemIntrinic is not implemented.

[hfinkel]

Do you have a test case? I don't understand from your description what's happening. Do you have a target intrinsic with an ID in the wrong range (i.e., not > FIRST_TARGET_MEMORY_OPCODE)?

[arsenm]

In D41083#955697, @hfinkel wrote:

Do you have a test case? I don't understand from your description what's happening. Do you have a target intrinsic with an ID in the wrong range (i.e., not > FIRST_TARGET_MEMORY_OPCODE)?

As it is now, if you have a raw intrinsic (i.e. ISD::INTRINSIC_*) node, the memory operand is always dropped. D40978 allows preserving it by setting the property on the intrinsic, so OPC_RecordMemRef will actually be encountered. If you have an intrinsic marked as having a memory operand, but getTgtMemIntrinsic does not handle it, it won't be a MemSDNode. The other case is probably a table gen bug not understanding different memory properties when the same opcode is used, but I'm not sure how worth it is continuing to investigate that.

[hfinkel]

Okay. Can you add a DEBUG statement saying something about when we're dropping the memory operand?

[arsenm]

Add some debug printing

[hfinkel]

LGTM

[arsenm]

r321208

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

case OPC_RecordChild6: case OPC_RecordChild7: {
       if (ChildNo >= N.getNumOperands())
         break;  // Match fails if out of range child #.
 
       RecordedNodes.push_back(std::make_pair(N->getOperand(ChildNo),
                                              N.getNode()));
       continue;
     }
     case OPC_RecordMemRef:
-      MatchedMemRefs.push_back(cast<MemSDNode>(N)->getMemOperand());
+      if (auto *MN = dyn_cast<MemSDNode>(N))
+        MatchedMemRefs.push_back(MN->getMemOperand());
+      else {
+        DEBUG(
+          dbgs() << "Expected MemSDNode ";
+          N->dump(CurDAG);
+          dbgs() << '\n'
+        );
+      }
+
       continue;
 
     case OPC_CaptureGlueInput:
       // If the current node has an input glue, capture it in InputGlue.
       if (N->getNumOperands() != 0 &&
           N->getOperand(N->getNumOperands()-1).getValueType() == MVT::Glue)
         InputGlue = N->getOperand(N->getNumOperands()-1);
       continue;
case OPC_MorphNodeTo0: case OPC_MorphNodeTo1: case OPC_MorphNodeTo2: {
 
       // If this has chain/glue inputs, add them.
       if (EmitNodeInfo & OPFL_Chain)
         Ops.push_back(InputChain);
       if ((EmitNodeInfo & OPFL_GlueInput) && InputGlue.getNode() != nullptr)
         Ops.push_back(InputGlue);
 
       // Create the node.
-      SDNode *Res = nullptr;
+      MachineSDNode *Res = nullptr;
       bool IsMorphNodeTo = Opcode == OPC_MorphNodeTo ||
                      (Opcode >= OPC_MorphNodeTo0 && Opcode <= OPC_MorphNodeTo2);
       if (!IsMorphNodeTo) {
         // If this is a normal EmitNode command, just create the new node and
         // add the results to the RecordedNodes list.
         Res = CurDAG->getMachineNode(TargetOpc, SDLoc(NodeToMatch),
                                      VTList, Ops);
 
case OPC_MorphNodeTo0: case OPC_MorphNodeTo1: case OPC_MorphNodeTo2: {
         SelectionDAG::DAGNodeDeletedListener NDL(*CurDAG, [&](SDNode *N,
                                                               SDNode *E) {
           CurDAG->salvageDebugInfo(*N);
           auto &Chain = ChainNodesMatched;
           assert((!E || !is_contained(Chain, N)) &&
                  "Chain node replaced during MorphNode");
           Chain.erase(std::remove(Chain.begin(), Chain.end(), N), Chain.end());
         });
-        Res = MorphNode(NodeToMatch, TargetOpc, VTList, Ops, EmitNodeInfo);
+        Res = cast<MachineSDNode>(MorphNode(NodeToMatch, TargetOpc, VTList,
+                                            Ops, EmitNodeInfo));
       }
 
       // If the node had chain/glue results, update our notion of the current
       // chain and glue.
       if (EmitNodeInfo & OPFL_GlueOutput) {
         InputGlue = SDValue(Res, VTs.size()-1);
         if (EmitNodeInfo & OPFL_Chain)
           InputChain = SDValue(Res, VTs.size()-2);
case OPC_MorphNodeTo0: case OPC_MorphNodeTo1: case OPC_MorphNodeTo2: {
           } else if ((*I)->isStore()) {
             if (mayStore)
               *MemRefsPos++ = *I;
           } else {
             *MemRefsPos++ = *I;
           }
         }
 
-        cast<MachineSDNode>(Res)
-          ->setMemRefs(MemRefs, MemRefs + NumMemRefs);
+        //Res->setMemRefs(MemRefs, MemRefs + NumMemRefs);
       }
 
-      DEBUG(dbgs() << "  "
-                   << (IsMorphNodeTo ? "Morphed" : "Created")
-                   << " node: "; Res->dump(CurDAG); dbgs() << "\n");
+      DEBUG(
+        if (!MatchedMemRefs.empty() && Res->memoperands_empty())
+          dbgs() << "  Dropping mem operands\n";
+        dbgs() << "  "
+               << " node: ";
+        Res->dump(CurDAG);
+
+        dbgs() << '\n';
+      );
 
       // If this was a MorphNodeTo then we're completely done!
       if (IsMorphNodeTo) {
         // Update chain uses.
         UpdateChains(Res, InputChain, ChainNodesMatched, true);
         return;
       }
       continue;