[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:11637
+    if (TargetType == MVT::v8i16) {
+      if (PreExtendVT != MVT::v8i8 && PreExtendVT != MVT::v16i8)
+        return false;
----------------
dmgreen wrote:
> 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.
Not entirely sure it is, I added that check to explicitly allow the types accepted by u/smull. I've changed this now.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11681
+      DAG.getVectorShuffle(PreExtendVT, DebugLoc, InsertVectorNode,
+                           DAG.getUNDEF(PreExtendVT), ShuffleMask);
+
----------------
dmgreen wrote:
> 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.
Generating the DUP directly, while simpler at this point, failed due to this combine being performed fairly early, and the DUP not being handled as part of lowering. Given that, keeping it as shuffle_vector at this point seems simpler, as we can also benefit from the existing VECTOR_SHUFFLE->DUP lowering checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91255