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

[Craig Topper via Phabricator via llvm-commits]

craig.topper marked 3 inline comments as done.
craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:145-148
+  std::pair<SDValue, SDValue>  ExpandUADDSUBO(SDValue Op);
+  std::pair<SDValue, SDValue>  ExpandSADDSUBO(SDValue Op);
+  std::pair<SDValue, SDValue>  ExpandMULO(SDValue Op);
   SDValue ExpandAddSubSat(SDValue Op);
----------------
arsenm wrote:
> Extra space here.
> 
> Is there a real point to returning SDValues here instead of just the SDNode*? I assume this always returns (N, 0), (N, 1)
The handlers are calling out to TargetLowering and that's giving back two separate SDValues. I think they are different nodes. For example for UADDO, I think one result would be from an ADD and one would be from a SETCC.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:495
+        AddLegalizedOperand(Op, Tmp);
+        return Tmp;
+      }
----------------
kpn wrote:
> Is this a new case? Or was this a situation that could occur before your changes but was too subtle to see?
It causes the assert below to fail in some cases. We never had the assert before and we only ever put one of the results from the custom handling into the map. You'll see there's a similar 1 result check in LegalizeDAG as well.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:502
+             "Unexpected number of results");
+      return TranslateLegalizeResults(Op, Tmp.getNode());
     }
----------------
kpn wrote:
> It seems like a code smell to have the rules duplicated in the Promote, Expand, and Custom cases. Is there a way to pull most of this block out into a function shared by the three cases? And yet the Custom case looks a little different from the others. 
Custom is a bit nasty due to only returning one SDValue. I had to rework it again for D72238. I should probably move that part of the change over here as I'm not entirely sure the changes I had to make there aren't needed for other cases.

I'll look into merging the handling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72224