[PATCH] D128065: [AArch64][SVE] Fold target specific ext/trunc nodes into loads/stores

[Bradley Smith via Phabricator via llvm-commits]

bsmith added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17523-17530
+      Value.getValueType().isInteger()) {
+    Value = Value.getOperand(0);
+    if (Value.getOpcode() == ISD::BITCAST) {
+      EVT HalfVT =
+          Value.getValueType().getHalfNumVectorElementsVT(*DAG.getContext());
+      EVT InVT = Value.getOperand(0).getValueType();
+
----------------
paulwalker-arm wrote:
> Whilst this works, you don't need to restrict the combine like this.  Here you're checking the stored elements all come from `UZP1`'s first operand.  You could do this using:
> ```
> ValueVT.isInteger() && ValueVT != MVT::nxv2i64) {
> EVT HalfVT = ValueVT.getHalfNumVectorElementsVT(*DAG.getContext());
> EVT InVT = HalfVT.widenIntegerVectorElementType(*DAG.getContext());
> ```
> followed by your current `NumElts` check. Then just before the `getMaskedStore()` create a fresh bitcast (e.g. `Value = bitcast(Value.getOperand(0) to InVT)`.  This means you don't really care what `UZP1`'s first operand is and you might catch more cases.
> 
I think for now this change should be left to a separate ticket, if we ever encounter this causing an issue. The logic around this is already quite confusing and I worry a change like that would make this section of code very confusing, whilst possibly not benefiting anything realistic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128065