[PATCH] D95967: [DAGCombiner][SVE] Fix invalid use of getVectorNumElements() in visitSRA.

[Huihui Zhang via Phabricator via llvm-commits]

huihuiz added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8364-8365
     if (VT.isVector())
-      ExtVT = EVT::getVectorVT(*DAG.getContext(),
-                               ExtVT, VT.getVectorNumElements());
+      ExtVT = EVT::getVectorVT(*DAG.getContext(), ExtVT,
+                               VT.getVectorElementCount());
     if (!LegalOperations ||
----------------
paulwalker-arm wrote:
> Just a thought but in general it seems like a good way to mitigate such issues is to remove any explicit vector length handling and thus I'm wondering if it's worth changing this and the other instances to use changeVectorElementType (i.e. `VT.changeVectorElementType(ExtVT)` in this instance). It might even reduce line wrapping and thus read better.
It's definitely nicer to use changeVectorElementType().
I tried to change it as suggested, but ran into a case I couldn't get over.

VT is v3i16, a valid simple value type, but ExtTy will be v3i1, not a valid simple integer vector type.

Please refer to test.ll , run with this patch and changeVectorElementType() change
llc -mtriple aarch64-none-linux-gnu < test.ll
Then you will see it assert with "Simple vector VT not representable by simple integer vector VT".

```
define <3 x i16> @sext_in_reg_v3i1_to_v3i16(<3 x i16> %a, <3 x i16> %b) {
  %c = add <3 x i16> %a, %b ;
  %shl = shl <3 x i16> %a, <i16 15, i16 15, i16 15>
  %ashr = ashr <3 x i16> %shl, <i16 15, i16 15, i16 15>
  ret <3 x i16> %ashr
}
```
( I found this test from test/CodeGen/AMDGPU/sext-in-reg.ll )

Also, we will need to restrict ExtTy/TruncTy to be simple type. In most cases VT is simple type, but with ashr (shl ...) TruncTy could be extended type (e.g., i7), then we will have null pointer access to LLVMTy when calling changeExtendedVectorElementType().



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8419
       if (VT.isVector())
-        TruncVT = EVT::getVectorVT(Ctx, TruncVT, VT.getVectorNumElements());
+        TruncVT = EVT::getVectorVT(Ctx, TruncVT, VT.getVectorElementCount());
 
----------------
david-arm wrote:
> Is it possible to add a test for this too?
Test @ashr_shl and @ashr_shl_illegal_trunc_vec_ty will hit this line.
Although I couldn't construct a test to make the folding happen. Subtraction of two splat value is not a constantSDNode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95967