[PATCH] D91255: [AArch64] Rearrange mul(dup(sext/zext)) to mul(sext/zext(dup))

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks, that makes it clearer at least.

There's quite a bit of code here. I was hoping this would be a lot simpler, and it would be good if some of this could be simplified (but a lot of this might well be needed for all the cases that are being supported?). It makes it more difficult to check all the things it's doing.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11565
+  std::function<SDValue(EVT)> BuildTypeNode = [&](EVT Type) {
+    return DAG.getNode(ISD::UNDEF, DebugLoc, Type, DAG.getValueType(Type));
+  };
----------------
There is a convenience function, DAG.getUNDEF(VT);

Probably doesn't need a lambda as a result.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11597
+      if (PreExtendVT != MVT::v8i8 && PreExtendVT != MVT::v16i8) {
+        LLVM_DEBUG(dbgs() << "Invalid type for vector extend, "
+                          << PreExtendVT.getEVTString()
----------------
Debug messages are fairly uncommon in ISel Lowering. It tends to trigger a lot of times where they are not important, and the debug messages it prints already are usually enough to give a rough indication of what is going on.

This whole function probably simplifies a lot I think? Is it something like TargetType.getHalfNumVectorElementsVT == PreExtendVT and the TargetType is one of {...}? I'm not sure how the MVT::v16i8 extended to MVT::v8i16 works out, from ISel. I think because ISel is type checked, those cases would not come up even if the underlying instructions supported it? We are not optimizing smull2 as far as I understand.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11628
+  if (SDValue VectorShuffleCombine =
+          performVectorShuffleExtCombine(Dup, DAG, LegalityFilter))
+    return VectorShuffleCombine;
----------------
Do we need to check both shuffles and DUP's? It that for the i64 types that are otherwise illegal somehow?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11653
+  // Non-integer vector types are unsupported
+  if (!TargetTypeVectorElement.isInteger())
+    return SDValue();
----------------
A mul should always be an integer I think?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11678-11679
+
+  // If the value type isn't a vector, none
+  // of the operands are going to be dups
+  if (!Mul->getValueType(0).isVector())
----------------
This comment is very short (as in - the length of the line). It probably doesn't need the newline before it either.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11686
+
+  for (unsigned i = 0; i < Mul->getNumOperands(); i++) {
+    const SDValue &Operand = Mul->getOperand(i);
----------------
Multiplies (and other BinOps) only have 2 operands. It would probably be simpler to just check both the operands as opposed to needing the loop.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11695
+
+  if (!Changed)
+    return SDValue();
----------------
This also seems to make changes without necessarily detecting that they are useful. That is probably fine here considering what is being transformed, but it can be better to represent it as "test if it will help, if so then make the change".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91255