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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

In D91255#2454474 <https://reviews.llvm.org/D91255#2454474>, @NickGuy wrote:

> Fixed formatting, and removed "Unsupported combines" tests as they were failing due to a separate issue (and contribute very little value to this patch)

It might be worth keeping one or two to show it doesn't do anything wrong/crash on incorrect types.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11628
+/// making use of the vector SExt/ZExt rather than the scalar SExt/ZExt
+static SDValue performCommonVectorExtendCombine(SDNode *VectorShuffle,
+                                                SelectionDAG &DAG) {
----------------
SDNode -> SDValue


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11637
+    if (TargetType == MVT::v8i16) {
+      if (PreExtendVT != MVT::v8i8 && PreExtendVT != MVT::v16i8)
+        return false;
----------------
I may be wrong, but is the `PreExtendVT != MVT::v16i8` half of this ever used? TargetType and PreExtendVT should have the same number of vector elements I believe. And then `TargetType.getScalarSizeInBits() == 2 * PreExtendVT.getScalarSizeInBits()` maybe?

That might help this lambda simplify further, possibly to the point that it is easier to inline it, now that it is only used in one place.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11670
+
+  SDLoc DebugLoc(VectorShuffle);
+
----------------
Nit: DebugLoc is usually called just DL (I think DebugLoc is already the name of the type used in Machine Instructions).


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11674
+      DAG.getNode(InsertVectorElt.getOpcode(), DebugLoc, PreExtendVT,
+                  {DAG.getUNDEF(PreExtendVT), Extend.getOperand(0),
+                   DAG.getConstant(0, DebugLoc, MVT::i64)});
----------------
Nit: It doesn't need the {} around the operands, there are overloaded methods to handle that automatically already.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11681
+      DAG.getVectorShuffle(PreExtendVT, DebugLoc, InsertVectorNode,
+                           DAG.getUNDEF(PreExtendVT), ShuffleMask);
+
----------------
It may be simpler to generate the AArch64ISD::DUP directly? I'm not sure either way which is better, but it's less nodes and we know it's going to get there eventually. Be careful about illegal types though.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11573
 
+static EVT calculatePreExtendVectorType(SDNode *Extend, EVT PostExtendType,
+                                        SelectionDAG &DAG) {
----------------
Add a message explaining what this function does. It looks generally useful.

It might also be worth having this function return the PreExtendType instead, and letting the parent function do the extend to vector type. Either way is fine but it might allow the function to be used for other things, if they come up.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11597
+    if (!Constant) {
+      LLVM_DEBUG(dbgs() << "Type operand is not a constant.\n");
+      return MVT::Other;
----------------
Can remove debug here and below.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11619
+    return MVT::Other;
+    break;
+  }
----------------
Nit: Doesn't need a break like this after a return.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11622
+
+  return PostExtendType.getVectorVT(*DAG.getContext(), PreExtendType,
+                                    PostExtendType.getVectorElementCount());
----------------
I think this can be PostExtendType.changeVectorElementType(PreExtendType);


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11686
+
+  for (unsigned i = 0; i < Mul->getNumOperands(); i++) {
+    const SDValue &Operand = Mul->getOperand(i);
----------------
NickGuy wrote:
> 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.
It would seem simpler (and smaller) to just call performCommonVectorExtendCombine for each operand. It may need to use something like `Op0 ? Op0 : Mul->getOperand(0)`, but it would remove the need to track Changed and most of the rest of this loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91255