This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner][SVE] Fix invalid use of getVectorNumElements() in visitSRA.
ClosedPublic

Authored by huihuiz on Feb 3 2021, 12:17 PM.

Details

Diff Detail

Event Timeline

huihuiz created this revision.Feb 3 2021, 12:17 PM
huihuiz requested review of this revision.Feb 3 2021, 12:17 PM

Take test function @sext_inreg
run: llc -mtriple=aarch64-linux-gnu -mattr=+sve < test/CodeGen/AArch64/DAGCombine_vscale.ll

Current upstream assert with "Assertion `(!EVT.isVector() || EVT.getVectorElementCount() == VT.getVectorElementCount()) && "Vector element counts must match in SIGN_EXTEND_INREG"' failed."

Hi, this patch looks like it has some good fixes, thanks! I just left one comment about adding a test if possible?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8419

Is it possible to add a test for this too?

paulwalker-arm added inline comments.Feb 4 2021, 3:34 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8364–8365

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.

huihuiz updated this revision to Diff 321629.Feb 4 2021, 6:30 PM
huihuiz marked an inline comment as done.

Thanks David and Paul for the reviews!

I have included test coverage for the second change of getVectorElementCount.

But when trying to use changeVectorElementType(), I ran into a case where VT is v3i16, and the computed ExtVT is v3i1 not a valid simple integer vector type. We might end up needing extra special handling for i1 and other types.
Please refer to detailed test case in the comments.

Let me know if it's ok to keep the usage of EVT::getVectorVT(..., ElementCount) ?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
8364–8365

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().

8419

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.

paulwalker-arm accepted this revision.Feb 5 2021, 3:24 AM
This revision is now accepted and ready to land.Feb 5 2021, 3:24 AM
This revision was landed with ongoing or failed builds.Feb 5 2021, 9:57 AM
This revision was automatically updated to reflect the committed changes.