This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombine] Check the legality of the index of EXTRACT_SUBVECTOR
ClosedPublic

Authored by guopeilin on Aug 9 2021, 6:38 PM.

Details

Summary

For ISD::EXTRACT_SUBVECTOR, its second operand must be a constant
multiple of the known-minimum vector length of the result type.

Diff Detail

Event Timeline

guopeilin created this revision.Aug 9 2021, 6:38 PM
guopeilin requested review of this revision.Aug 9 2021, 6:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2021, 6:38 PM
david-arm added inline comments.
llvm/test/CodeGen/AArch64/aarch64-avoid-illegal-extract-subvector.ll
9

Would it be possible to reduce this test a lot further? It looks like it shouldn't be too difficult to hand-write a new test that does something like:

%i1 = extractelement <4 x i32> %x, i32 1
%zi1 = zext i32 %i1 to i64
%i2 = extractelement <4 x i32> %x, i32 2
%zi2 = zext i32 %i2 to i64
%v1 = insertelement <2 x i64> undef, %zi1, i32 0
%v2 = insertelement <2 x i64> %v1, %zi2, i32 1

That looks like what the DAG combine is testing for and since the first extract is at index 1 it is not a multiple of the number of elements in the final build vector (<2 x i64>)

RKSimon added inline comments.Aug 17 2021, 8:09 AM
llvm/test/CodeGen/AArch64/aarch64-avoid-illegal-extract-subvector.ll
9

@guopeilin Any luck with simplifying this test?

guopeilin updated this revision to Diff 367100.Aug 17 2021, 8:36 PM

Hi all, I update a new patch with a much simple test case. Please review, Thanks a lot!

Pinging reviewers....

RKSimon added inline comments.Aug 23 2021, 6:21 AM
llvm/test/CodeGen/AArch64/aarch64-avoid-illegal-extract-subvector.ll
9

Can this be cleaned up anymore - the dso_local / local_unnamed_addr attributes seem superfluous for instance.

Do all the checks pass with this? The precommit checks suggest that an AMD test is failing.

llvm/test/CodeGen/AArch64/aarch64-avoid-illegal-extract-subvector.ll
9

Can you also generate the checks with the script. The file says it's generated with update_llc_test_checks, but these checks look sparse. Can you also add the testcase David talked about:

define <2 x i64> @test(<4 x i32> %x) #0 {
  %i1 = extractelement <4 x i32> %x, i32 1
  %zi1 = zext i32 %i1 to i64
  %i2 = extractelement <4 x i32> %x, i32 2
  %zi2 = zext i32 %i2 to i64
  %v1 = insertelement <2 x i64> undef, i64 %zi1, i32 0
  %v2 = insertelement <2 x i64> %v1, i64 %zi2, i32 1
  ret <2 x i64> %v2
}

It won't fail, but does show the same unaligned EXTRACT_SUBVECTOR. I guess that at some point worked for unaligned extract offsets.

guopeilin retitled this revision from [AArch64][DAGCombine] Check the legality of the index of EXTRACT_SUBVECTOR to [DAGCombine] Check the legality of the index of EXTRACT_SUBVECTOR.

Hi all!
Updata a new test case.
Function test2 will fail due to assertion of invalid offset of EXTRACT_SUBVECTOR.
Also modify the AMD test case

dmgreen accepted this revision.Aug 24 2021, 12:12 AM

Thanks. Looks good to me, as per the definition of EXTRACT_SUBVECTOR.

This revision is now accepted and ready to land.Aug 24 2021, 12:12 AM