[PATCH] D72224: [LegalizeVectorOps] Improve handling of multi-result operations.

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

In general this all makes sense to me. See a few minor inline comments.

I agree with @kpn's comment about code duplication.  I was wondering whether it might make sense to abstract some of that, even while the TLI.LowerOperation interface remains unchanged.  E.g. we could have a routine like this:

  bool CustomLower(SDNode *Node, SmallVectorImpl<SDValue> &Results) {
    if (SDValue Tmp = TLI.LowerOperation(SDValue(Node, 0), DAG)) {
      LLVM_DEBUG(dbgs() << "Successfully custom legalized node\n");
      if (Tmp == SDValue(Node, 0))
        return true;
      
      // Tmp might point to a single result from a multi result node, in that
      // case we need to use it's result number.
      if (Node->getNumValues() == 1) {
        Results.push_back(Tmp);
        return true;
      }
      
      // Otherwise it should be a multi-result node with the same number of
      // results.
      assert(Tmp->getNumValues() == Node->getNumValues() &&
             "Unexpected number of results");
      
      for (unsigned i = 0, e = Node->getNumValues(); i != e; ++i)
        Results.push_back(Tmp.getValue(i));
      return true;
    }
    
    LLVM_DEBUG(dbgs() << "Could not custom legalize node\n");
    return false;
  }

which could be used e.g. like so:

  SmallVector<SDValue, 8> ResultVals;
  if (CustomLower(Node, ResultVals)) {
    if (ResultVals.empty())
      return TranslateLegalizeResults(Op, Node);
  
    Changed = true;
    return RecursivelyLegalizeResults(Op, ResultVals);
  }
  LLVM_FALLTHROUGH;  // to expand

which would make the interfaces of the CustomLower / Expand / Promote cases the same, probably enabling further code sharing at the call site(s).



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:279
+          if (Lowered == SDValue(Node, 0))
+            return TranslateLegalizeResults(Op, Lowered.getNode());
+
----------------
Lowered.getNode() must always be Node here, right?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:509
+    TranslateLegalizeResults(Op, Node);
+    return SDValue(Node, Op.getResNo());
   case TargetLowering::Custom: {
----------------
Could just return the result of TranslateLegalizeResults, right?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:548
+      TranslateLegalizeResults(Op, Node);
+      return SDValue(Node, Op.getResNo());
+    }
----------------
Likewise, just return the result.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:884
+    if (SDValue Tmp = ExpandFSUB(Op))
+      Results.push_back(Tmp);
+    return;
----------------
It's a bit odd to hard-code that **some** expanders may return a null SDValue and others may not.  It may be preferable to just pass the Results vector through.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72224/new/

https://reviews.llvm.org/D72224