SDAG: Rename Select->SelectImpl and repurpose Select as returning void

This is a step towards removing the rampant undefined behaviour in
SelectionDAG, which is a part of llvm.org/PR26808.

We rename SelectionDAGISel::Select to SelectImpl and update targets to
match, and then change Select to return void and consolidate the
sketchy behaviour we're trying to get away from there.

Next, we'll update backends to implement `void Select(...)` instead of
SelectImpl and eventually drop the base Select implementation.

llvm-svn: 268693

llvm/docs/ReleaseNotes.rst

@@ -61,6 +61,10 @@ Non-comprehensive list of changes in this release
   iterator to the next instruction instead of ``void``. Targets that previously
   did ``MBB.erase(I); return;`` now probably want ``return MBB.erase(I);``.
 
+* ``SelectionDAGISel::Select`` now returns ``void``. Out of tree targets will
+  need to be updated to replace the argument node and remove any dead nodes in
+  cases where they currently return an ``SDNode *`` from this interface.
+
 .. NOTE
    For small 1-3 sentence descriptions, just add an entry at the end of
    this list. If your description won't fit comfortably in one bullet

llvm/include/llvm/CodeGen/SelectionDAGISel.h

@@ -76,8 +76,40 @@ public:
   /// right after selection.
   virtual void PostprocessISelDAG() {}
 
-  /// Select - Main hook targets implement to select a node.
-  virtual SDNode *Select(SDNode *N) = 0;
+  /// Main hook for targets to transform nodes into machine nodes.
+  ///
+  /// All targets should implement this hook. The default implementation will be
+  /// made abstract once all targets are migrated off of the legacy hook.
+  virtual void Select(SDNode *N) {
+    SDNode *New = SelectImpl(N);
+    // TODO: Checking DELETED_NODE here is undefined behaviour, which will be
+    // fixed by migrating backends to implement the void Select interface
+    // instead or returning a node.
+    if (New == N || N->getOpcode() == ISD::DELETED_NODE)
+      // If we ask to replace the node with itself or if we deleted the original
+      // node, just move on to the next one. This case will go away once
+      // everyone migrates to stop implementing SelectImpl.
+      return;
+    if (New) {
+      // Replace the node with the returned node. Originally, Select would
+      // always return a node and the caller would replace it, but this doesn't
+      // work for more complicated selection schemes.
+      ReplaceUses(N, New);
+      CurDAG->RemoveDeadNode(N);
+    } else if (N->use_empty())
+      // Clean up dangling nodes if the target didn't bother. These are
+      // basically bugs in the targets, but we were lenient in the past and did
+      // this for them.
+      CurDAG->RemoveDeadNode(N);
+  }
+
+  /// Legacy hook to support transitioning to the return-less Select().
+  ///
+  /// This exposes the old style Select hook. New code should implement void
+  /// Select() instead.
+  virtual SDNode *SelectImpl(SDNode *N) {
+    llvm_unreachable("Subclasses must implement one of Select or SelectImpl");
+  }
 
   /// SelectInlineAsmMemoryOperand - Select the specified address as a target
   /// addressing mode, according to the specified constraint.  If this does

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

@@ -950,23 +950,7 @@ void SelectionDAGISel::DoInstructionSelection() {
       if (Node->use_empty())
         continue;
 
-      SDNode *ResNode = Select(Node);
-
-      // FIXME: This is pretty gross.  'Select' should be changed to not return
-      // anything at all and this code should be nuked with a tactical strike.
-
-      // If node should not be replaced, continue with the next one.
-      if (ResNode == Node || Node->getOpcode() == ISD::DELETED_NODE)
-        continue;
-      // Replace node.
-      if (ResNode) {
-        ReplaceUses(Node, ResNode);
-      }
-
-      // If after the replacement this node is not used any more,
-      // remove this dead node.
-      if (Node->use_empty()) // Don't delete EntryToken, etc.
-        CurDAG->RemoveDeadNode(Node);
+      Select(Node);
     }
 
     CurDAG->setRoot(Dummy.getValue());

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

@@ -57,7 +57,7 @@ public:
     return SelectionDAGISel::runOnMachineFunction(MF);
   }
 
-  SDNode *Select(SDNode *Node) override;
+  SDNode *SelectImpl(SDNode *Node) override;
 
   /// SelectInlineAsmMemoryOperand - Implement addressing mode selection for
   /// inline asm expressions.
@@ -2329,7 +2329,7 @@ void AArch64DAGToDAGISel::SelectCMP_SWAP(SDNode *N) {
   ReplaceUses(SDValue(N, 1), SDValue(CmpSwap, 2));
 }
 
-SDNode *AArch64DAGToDAGISel::Select(SDNode *Node) {
+SDNode *AArch64DAGToDAGISel::SelectImpl(SDNode *Node) {
   // Dump information about the Node being selected
   DEBUG(errs() << "Selecting: ");
   DEBUG(Node->dump(CurDAG));

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

@@ -61,7 +61,7 @@ public:
   AMDGPUDAGToDAGISel(TargetMachine &TM);
   virtual ~AMDGPUDAGToDAGISel();
   bool runOnMachineFunction(MachineFunction &MF) override;
-  SDNode *Select(SDNode *N) override;
+  SDNode *SelectImpl(SDNode *N) override;
   const char *getPassName() const override;
   void PreprocessISelDAG() override;
   void PostprocessISelDAG() override;
@@ -329,7 +329,7 @@ static unsigned selectSGPRVectorRegClassID(unsigned NumVectorElts) {
   llvm_unreachable("invalid vector size");
 }
 
-SDNode *AMDGPUDAGToDAGISel::Select(SDNode *N) {
+SDNode *AMDGPUDAGToDAGISel::SelectImpl(SDNode *N) {
   unsigned int Opc = N->getOpcode();
   if (N->isMachineOpcode()) {
     N->setNodeId(-1);

llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp

@@ -87,8 +87,7 @@ public:
     return CurDAG->getTargetConstant(Imm, dl, MVT::i32);
   }
 
-  SDNode *Select(SDNode *N) override;
-
+  SDNode *SelectImpl(SDNode *N) override;
 
   bool hasNoVMLxHazardUse(SDNode *N) const;
   bool isShifterOpProfitable(const SDValue &Shift,
@@ -2634,7 +2633,7 @@ SDNode *ARMDAGToDAGISel::SelectConcatVector(SDNode *N) {
   return createDRegPairNode(VT, N->getOperand(0), N->getOperand(1));
 }
 
-SDNode *ARMDAGToDAGISel::Select(SDNode *N) {
+SDNode *ARMDAGToDAGISel::SelectImpl(SDNode *N) {
   SDLoc dl(N);
 
   if (N->isMachineOpcode()) {

llvm/lib/Target/BPF/BPFISelDAGToDAG.cpp

@@ -46,7 +46,7 @@ private:
 // Include the pieces autogenerated from the target description.
 #include "BPFGenDAGISel.inc"
 
-  SDNode *Select(SDNode *N) override;
+  SDNode *SelectImpl(SDNode *N) override;
 
   // Complex Pattern for address selection.
   bool SelectAddr(SDValue Addr, SDValue &Base, SDValue &Offset);
@@ -115,7 +115,7 @@ bool BPFDAGToDAGISel::SelectFIAddr(SDValue Addr, SDValue &Base, SDValue &Offset)
   return false;
 }
 
-SDNode *BPFDAGToDAGISel::Select(SDNode *Node) {
+SDNode *BPFDAGToDAGISel::SelectImpl(SDNode *Node) {
   unsigned Opcode = Node->getOpcode();
 
   // Dump information about the Node being selected

llvm/lib/Target/Hexagon/HexagonISelDAGToDAG.cpp

@@ -70,7 +70,7 @@ public:
   virtual void PreprocessISelDAG() override;
   virtual void EmitFunctionEntryCode() override;
 
-  SDNode *Select(SDNode *N) override;
+  SDNode *SelectImpl(SDNode *N) override;
 
   // Complex Pattern Selectors.
   inline bool SelectAddrGA(SDValue &N, SDValue &R);
@@ -1284,7 +1284,7 @@ SDNode *HexagonDAGToDAGISel::SelectFrameIndex(SDNode *N) {
 }
 
 
-SDNode *HexagonDAGToDAGISel::Select(SDNode *N) {
+SDNode *HexagonDAGToDAGISel::SelectImpl(SDNode *N) {
   if (N->isMachineOpcode()) {
     N->setNodeId(-1);
     return nullptr;   // Already selected.

llvm/lib/Target/Lanai/LanaiISelDAGToDAG.cpp

@@ -68,7 +68,7 @@ private:
 #include "LanaiGenDAGISel.inc"
 
   // Instruction Selection not handled by the auto-generated tablgen
-  SDNode *Select(SDNode *N) override;
+  SDNode *SelectImpl(SDNode *N) override;
 
   // Support functions for the opcodes of Instruction Selection
   // not handled by the auto-generated tablgen
@@ -270,7 +270,7 @@ bool LanaiDAGToDAGISel::SelectInlineAsmMemoryOperand(
 
 // Select instructions not customized! Used for
 // expanded, promoted and normal instructions
-SDNode *LanaiDAGToDAGISel::Select(SDNode *Node) {
+SDNode *LanaiDAGToDAGISel::SelectImpl(SDNode *Node) {
   unsigned Opcode = Node->getOpcode();
 
   // Dump information about the Node being selected

llvm/lib/Target/MSP430/MSP430ISelDAGToDAG.cpp

@@ -110,7 +110,7 @@ namespace {
   #include "MSP430GenDAGISel.inc"
 
   private:
-    SDNode *Select(SDNode *N) override;
+    SDNode *SelectImpl(SDNode *N) override;
     SDNode *SelectIndexedLoad(SDNode *Op);
     SDNode *SelectIndexedBinOp(SDNode *Op, SDValue N1, SDValue N2,
                                unsigned Opc8, unsigned Opc16);
@@ -376,7 +376,7 @@ SDNode *MSP430DAGToDAGISel::SelectIndexedBinOp(SDNode *Op,
 }
 
 
-SDNode *MSP430DAGToDAGISel::Select(SDNode *Node) {
+SDNode *MSP430DAGToDAGISel::SelectImpl(SDNode *Node) {
   SDLoc dl(Node);
 
   // Dump information about the Node being selected

llvm/lib/Target/Mips/MipsISelDAGToDAG.cpp

@@ -182,7 +182,7 @@ bool MipsDAGToDAGISel::selectVSplatMaskR(SDValue N, SDValue &Imm) const {
 
 /// Select instructions not customized! Used for
 /// expanded, promoted and normal instructions
-SDNode* MipsDAGToDAGISel::Select(SDNode *Node) {
+SDNode *MipsDAGToDAGISel::SelectImpl(SDNode *Node) {
   unsigned Opcode = Node->getOpcode();
 
   // Dump information about the Node being selected

llvm/lib/Target/Mips/MipsISelDAGToDAG.h

@@ -114,7 +114,7 @@ private:
   /// starting at bit zero.
   virtual bool selectVSplatMaskR(SDValue N, SDValue &Imm) const;
 
-  SDNode *Select(SDNode *N) override;
+  SDNode *SelectImpl(SDNode *N) override;
 
   virtual std::pair<bool, SDNode*> selectNode(SDNode *Node) = 0;
 

llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp

@@ -105,7 +105,7 @@ bool NVPTXDAGToDAGISel::allowFMA() const {
 
 /// Select - Select instructions not customized! Used for
 /// expanded, promoted and normal instructions.
-SDNode *NVPTXDAGToDAGISel::Select(SDNode *N) {
+SDNode *NVPTXDAGToDAGISel::SelectImpl(SDNode *N) {
 
   if (N->isMachineOpcode()) {
     N->setNodeId(-1);

llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h

@@ -53,7 +53,7 @@ private:
 // Include the pieces autogenerated from the target description.
 #include "NVPTXGenDAGISel.inc"
 
-  SDNode *Select(SDNode *N) override;
+  SDNode *SelectImpl(SDNode *N) override;
   SDNode *SelectIntrinsicNoChain(SDNode *N);
   SDNode *SelectIntrinsicChain(SDNode *N);
   SDNode *SelectTexSurfHandle(SDNode *N);
@@ -69,7 +69,7 @@ private:
   SDNode *SelectTextureIntrinsic(SDNode *N);
   SDNode *SelectSurfaceIntrinsic(SDNode *N);
   SDNode *SelectBFE(SDNode *N);
-        
+
   inline SDValue getI32Imm(unsigned Imm, SDLoc DL) {
     return CurDAG->getTargetConstant(Imm, DL, MVT::i32);
   }

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

@@ -126,7 +126,7 @@ namespace {
 
     // Select - Convert the specified operand from a target-independent to a
     // target-specific node if it hasn't already been changed.
-    SDNode *Select(SDNode *N) override;
+    SDNode *SelectImpl(SDNode *N) override;
 
     SDNode *SelectBitfieldInsert(SDNode *N);
     SDNode *SelectBitPermutation(SDNode *N);
@@ -1209,7 +1209,7 @@ class BitPermutationSelector {
                  "bit group ends at index 63 but there is another?");
           auto IN = BitGroups.begin();
 
-          if (IP->Repl32 && IN->Repl32 && I->V == IP->V && I->V == IN->V && 
+          if (IP->Repl32 && IN->Repl32 && I->V == IP->V && I->V == IN->V &&
               (I->RLAmt % 32) == IP->RLAmt && (I->RLAmt % 32) == IN->RLAmt &&
               IP->EndIdx == 31 && IN->StartIdx == 0 && I != IP &&
               IsAllLow32(*I)) {
@@ -2408,7 +2408,7 @@ SDNode *PPCDAGToDAGISel::transferMemOperands(SDNode *N, SDNode *Result) {
 
 // Select - Convert the specified operand from a target-independent to a
 // target-specific node if it hasn't already been changed.
-SDNode *PPCDAGToDAGISel::Select(SDNode *N) {
+SDNode *PPCDAGToDAGISel::SelectImpl(SDNode *N) {
   SDLoc dl(N);
   if (N->isMachineOpcode()) {
     N->setNodeId(-1);
@@ -4384,4 +4384,3 @@ static void initializePassOnce(PassRegistry &Registry) {
 void llvm::initializePPCDAGToDAGISelPass(PassRegistry &Registry) {
   CALL_ONCE_INITIALIZATION(initializePassOnce);
 }
-

llvm/lib/Target/Sparc/SparcISelDAGToDAG.cpp

@@ -41,7 +41,7 @@ public:
     return SelectionDAGISel::runOnMachineFunction(MF);
   }
 
-  SDNode *Select(SDNode *N) override;
+  SDNode *SelectImpl(SDNode *N) override;
 
   // Complex Pattern Selectors.
   bool SelectADDRrr(SDValue N, SDValue &R1, SDValue &R2);
@@ -317,7 +317,7 @@ SDNode *SparcDAGToDAGISel::SelectInlineAsm(SDNode *N){
   return New.getNode();
 }
 
-SDNode *SparcDAGToDAGISel::Select(SDNode *N) {
+SDNode *SparcDAGToDAGISel::SelectImpl(SDNode *N) {
   SDLoc dl(N);
   if (N->isMachineOpcode()) {
     N->setNodeId(-1);

llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp

@@ -343,7 +343,7 @@ public:
   }
 
   // Override SelectionDAGISel.
-  SDNode *Select(SDNode *Node) override;
+  SDNode *SelectImpl(SDNode *Node) override;
   bool SelectInlineAsmMemoryOperand(const SDValue &Op, unsigned ConstraintID,
                                     std::vector<SDValue> &OutOps) override;
 
@@ -1041,7 +1041,8 @@ SDNode *SystemZDAGToDAGISel::splitLargeImmediate(unsigned Opcode, SDNode *Node,
   SDValue Upper = CurDAG->getConstant(UpperVal, DL, VT);
   if (Op0.getNode())
     Upper = CurDAG->getNode(Opcode, DL, VT, Op0, Upper);
-  Upper = SDValue(Select(Upper.getNode()), 0);
+  // TODO: This is pretty strange. Not sure what it's trying to do...
+  Upper = SDValue(SelectImpl(Upper.getNode()), 0);
 
   SDValue Lower = CurDAG->getConstant(LowerVal, DL, VT);
   SDValue Or = CurDAG->getNode(Opcode, DL, VT, Upper, Lower);
@@ -1171,7 +1172,7 @@ bool SystemZDAGToDAGISel::storeLoadCanUseBlockBinary(SDNode *N,
   return !LoadA->isVolatile() && canUseBlockOperation(StoreA, LoadB);
 }
 
-SDNode *SystemZDAGToDAGISel::Select(SDNode *Node) {
+SDNode *SystemZDAGToDAGISel::SelectImpl(SDNode *Node) {
   // Dump information about the Node being selected
   DEBUG(errs() << "Selecting: "; Node->dump(CurDAG); errs() << "\n");
 

llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp

@@ -54,7 +54,7 @@ public:
     return SelectionDAGISel::runOnMachineFunction(MF);
   }
 
-  SDNode *Select(SDNode *Node) override;
+  SDNode *SelectImpl(SDNode *Node) override;
 
   bool SelectInlineAsmMemoryOperand(const SDValue &Op, unsigned ConstraintID,
                                     std::vector<SDValue> &OutOps) override;
@@ -67,7 +67,7 @@ private:
 };
 } // end anonymous namespace
 
-SDNode *WebAssemblyDAGToDAGISel::Select(SDNode *Node) {
+SDNode *WebAssemblyDAGToDAGISel::SelectImpl(SDNode *Node) {
   // Dump information about the Node being selected.
   DEBUG(errs() << "Selecting: ");
   DEBUG(Node->dump(CurDAG));

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp

@@ -196,7 +196,7 @@ namespace {
 #include "X86GenDAGISel.inc"
 
   private:
-    SDNode *Select(SDNode *N) override;
+    SDNode *SelectImpl(SDNode *N) override;
     SDNode *selectGather(SDNode *N, unsigned Opc);
 
     bool foldOffsetIntoAddress(uint64_t Offset, X86ISelAddressMode &AM);
@@ -326,7 +326,7 @@ namespace {
         // types.
         if (User->getNumOperands() != 2)
           continue;
-        
+
         // Immediates that are used for offsets as part of stack
         // manipulation should be left alone. These are typically
         // used to indicate SP offsets for argument passing and
@@ -1926,7 +1926,7 @@ SDNode *X86DAGToDAGISel::selectGather(SDNode *Node, unsigned Opc) {
   return ResNode;
 }
 
-SDNode *X86DAGToDAGISel::Select(SDNode *Node) {
+SDNode *X86DAGToDAGISel::SelectImpl(SDNode *Node) {
   MVT NVT = Node->getSimpleValueType(0);
   unsigned Opc, MOpc;
   unsigned Opcode = Node->getOpcode();

llvm/lib/Target/XCore/XCoreISelDAGToDAG.cpp

@@ -41,7 +41,7 @@ namespace {
     XCoreDAGToDAGISel(XCoreTargetMachine &TM, CodeGenOpt::Level OptLevel)
       : SelectionDAGISel(TM, OptLevel) {}
 
-    SDNode *Select(SDNode *N) override;
+    SDNode *SelectImpl(SDNode *N) override;
     SDNode *SelectBRIND(SDNode *N);
 
     /// getI32Imm - Return a target constant with the specified value, of type
@@ -69,14 +69,14 @@ namespace {
 
     const char *getPassName() const override {
       return "XCore DAG->DAG Pattern Instruction Selection";
-    } 
-    
+    }
+
     // Include the pieces autogenerated from the target description.
   #include "XCoreGenDAGISel.inc"
   };
 }  // end anonymous namespace
 
-/// createXCoreISelDag - This pass converts a legalized DAG into a 
+/// createXCoreISelDag - This pass converts a legalized DAG into a
 /// XCore-specific DAG, ready for instruction scheduling.
 ///
 FunctionPass *llvm::createXCoreISelDag(XCoreTargetMachine &TM,
@@ -129,7 +129,7 @@ SelectInlineAsmMemoryOperand(const SDValue &Op, unsigned ConstraintID,
   return false;
 }
 
-SDNode *XCoreDAGToDAGISel::Select(SDNode *N) {
+SDNode *XCoreDAGToDAGISel::SelectImpl(SDNode *N) {
   SDLoc dl(N);
   switch (N->getOpcode()) {
   default: break;
@@ -204,7 +204,7 @@ SDNode *XCoreDAGToDAGISel::Select(SDNode *N) {
 
 /// Given a chain return a new chain where any appearance of Old is replaced
 /// by New. There must be at most one instruction between Old and Chain and
-/// this instruction must be a TokenFactor. Returns an empty SDValue if 
+/// this instruction must be a TokenFactor. Returns an empty SDValue if
 /// these conditions don't hold.
 static SDValue
 replaceInChain(SelectionDAG *CurDAG, SDValue Chain, SDValue Old, SDValue New)