[PATCH] D44585: [AMDGPU] Scalarize when scalar code cheaper than vector code.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

There are no test changes in any other target?



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12806
+/// operation.
+static bool cheaperToScalarize(const SDValue &N, const SelectionDAG &DAG) {
+  if (!N.getValueType().isVector())
----------------
You can just pass in TLI


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12816-12817
+  if (N.getNumOperands() == 2) {
+    if (cheaperToScalarize(N.getOperand(0), DAG) ||
+        cheaperToScalarize(N.getOperand(1), DAG))
+      return true;
----------------
The recursive call still seems wrong to me. The fact that there are 2 elements and the vector shuffle will be split seems sufficient


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14315
 
+  // Transform: (EXTRACT_VECTOR_ELT( BIN_OP VECTOR_SHUFFLE )) -> EXTRACT_VECTOR_ELT.
+  if (ConstEltNo &&
----------------
Comment seems incomplete. These comments usually use some kind of variable name for the operands and repeat them in the output


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14318
+      InVec.getNumOperands() == 2 &&
+      TLI.isTypeLegal(VT) &&
+      InVec.hasOneUse()) {
----------------
Don't think the legality of the types matters


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14322-14327
+    case ISD::FADD:
+    case ISD::FSUB:
+    case ISD::FMUL:
+    case ISD::ADD:
+    case ISD::SUB:
+    case ISD::MUL:
----------------
I'm not sure why the white list of operations is necessary


https://reviews.llvm.org/D44585