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

[Nicholas Guy via Phabricator via llvm-commits]

NickGuy added inline comments.


================
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));
+  };
----------------
dmgreen wrote:
> There is a convenience function, DAG.getUNDEF(VT);
> 
> Probably doesn't need a lambda as a result.
Ah, thanks for pointing that out. 
Lambda is removed, too.


================
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()
----------------
dmgreen wrote:
> 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.
> Debug messages are fairly uncommon in ISel Lowering
Removed the debug messages.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11628
+  if (SDValue VectorShuffleCombine =
+          performVectorShuffleExtCombine(Dup, DAG, LegalityFilter))
+    return VectorShuffleCombine;
----------------
dmgreen wrote:
> Do we need to check both shuffles and DUP's? It that for the i64 types that are otherwise illegal somehow?
No, we don't. Looks like checking for shuffles catches the same DUP cases before they become DUPs. I've removed the DUP-specific check, and have unified it down the VectorShuffle check


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11653
+  // Non-integer vector types are unsupported
+  if (!TargetTypeVectorElement.isInteger())
+    return SDValue();
----------------
dmgreen wrote:
> A mul should always be an integer I think?
That was a remnant of before we were restricting this to muls only. Regardless, this has now been removed.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11686
+
+  for (unsigned i = 0; i < Mul->getNumOperands(); i++) {
+    const SDValue &Operand = Mul->getOperand(i);
----------------
dmgreen wrote:
> 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.
> It would probably be simpler to just check both the operands as opposed to needing the loop.
I disagree. Having the loop here makes it clearer that each operand is being handled in the same way, while having them separated needlessly duplicates the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91255