[PATCH] D120018: [AArch64] Alter mull shuffle(ext(..)) combine to work on buildvectors

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13512
-/// Combines a dup(sext/zext) node pattern into sext/zext(dup)
-/// making use of the vector SExt/ZExt rather than the scalar SExt/ZExt
-static SDValue performCommonVectorExtendCombine(SDValue VectorShuffle,
----------------
NickGuy wrote:
> Do we want to keep this comment (or replace it, rather than remove it)?
Oh yeah I can add this back in. I previously had both performCommonVectorExtendCombine and performBuildVectorExtendCombine methods being called, but removing the shuffle version made no effect on any of the tests we have if I removed it. The methods are still quite similar in places.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13542-13543
+  EVT PreExtendVT = VT.changeVectorElementType(PreExtendType);
+  EVT PreExtendLegalType =
+      PreExtendType.getScalarSizeInBits() < 32 ? MVT::i32 : PreExtendType;
+  SDLoc DL(BV);
----------------
NickGuy wrote:
> Is this correct? The previous behaviour used `PreExtendType` directly as an operand to `DAG.getAnyExtOrTrunc` (which kept it aligned with `PreExtendVT`), but this clamps it to a smallest type of i32.
It needs to be a legal type, so we can't just create i8 or i16 out of nowhere. The BUILD_VECTOR can be made up of elements that are implicitly truncated.
```
  /// BUILD_VECTOR(ELT0, ELT1, ELT2, ELT3,...) - Return a fixed-width vector
  /// with the specified, possibly variable, elements. The types of the
  /// operands must match the vector element type, except that integer types
  /// are allowed to be larger than the element type, in which case the
  /// operands are implicitly truncated. The types of the operands must all
  /// be the same.
```


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

https://reviews.llvm.org/D120018