This is an archive of the discontinued LLVM Phabricator instance.

Differential D146540

[SLP] Add test to check for GEP vectorization
ClosedPublic

Authored by zjaffal on Mar 21 2023, 9:21 AM.

Details

Reviewers
fhahn
Commits
rG984b46e6cc2a: [SLP] Add test to check for GEP vectorization
Summary

add a test to check for gep vectorization after the change from D144128 where the gep vectorization is dependant on the target hook prefersVectorizedAddressing()

Diff Detail

Event Timeline

zjaffal created this revision.Mar 21 2023, 9:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2023, 9:22 AM
Herald added subscribers: vporpo, ctetreau, StephenFan. · View Herald Transcript
zjaffal requested review of this revision.Mar 21 2023, 9:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2023, 9:22 AM
Herald added subscribers: llvm-commits, • pcwang-thead. · View Herald Transcript
Harbormaster completed remote builds in B220749: Diff 507017.Mar 21 2023, 10:52 AM
fhahn requested changes to this revision.Mar 22 2023, 3:45 AM

Looks like the test is failing in the precommit checks.

llvm/test/Transforms/SLPVectorizer/AArch64/getelementptr.ll
6 ↗(On Diff #507017)

Why is the new run line needed?

This revision now requires changes to proceed.Mar 22 2023, 3:45 AM
zjaffal updated this revision to Diff 507392.Mar 22 2023, 9:21 AM

Move function from AArch64 to be a generic test.

zjaffal updated this revision to Diff 507403.Mar 22 2023, 9:45 AM

move the file back to AArch64

Harbormaster completed remote builds in B221054: Diff 507403.Mar 22 2023, 10:55 AM
fhahn added inline comments.Mar 27 2023, 5:11 AM
llvm/test/Transforms/SLPVectorizer/AArch64/vector-getelementptr.ll
3

this is missing an AArch64 triple, you need to add something like -mtriple=arm64-apple-ios.

5

Please add a description to the test. The key issue is that it requires a vector GEP + extracts, but the cost is offset by being able to efficiently vectorize the rest of the tree.

44

please don't use null in the test. e.g. loading from null like the test does is UB.

zjaffal updated this revision to Diff 508635.Mar 27 2023, 6:53 AM

rename variables, change ptr from null to function argument and specify the mtriple for opt

zjaffal marked 3 inline comments as done.Mar 27 2023, 6:54 AM
zjaffal added inline comments.
llvm/test/Transforms/SLPVectorizer/AArch64/getelementptr.ll
6 ↗(On Diff #507017)

because -slp-threshold=-7 forces the test to vectorize I guess maybe it modifies the cost model?

Harbormaster completed remote builds in B221999: Diff 508635.Mar 27 2023, 7:58 AM
fhahn accepted this revision.Mar 27 2023, 8:35 AM

LGTM, thanks. Just a few bits that would be good to address before landing

llvm/test/Transforms/SLPVectorizer/AArch64/vector-getelementptr.ll
4

Move this directly to the test definition, with a newline between comment and run line

6

More consistent to use base3 instead of base_gep? All bases are used with geps

66

It would be more compact to use gep instead of get elementary in the names.

This revision is now accepted and ready to land.Mar 27 2023, 8:35 AM
Closed by commit rG984b46e6cc2a: [SLP] Add test to check for GEP vectorization (authored by zjaffal). · Explain WhyMar 27 2023, 9:53 AM
This revision was automatically updated to reflect the committed changes.
zjaffal added a commit: rG984b46e6cc2a: [SLP] Add test to check for GEP vectorization.

Revision Contents

PathSize
llvm/
test/
Transforms/
SLPVectorizer/
AArch64/
77 lines

Diff 508714

llvm/test/Transforms/SLPVectorizer/AArch64/vector-getelementptr.ll

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
; RUN: opt -mtriple=arm64-apple-ios -S -passes=slp-vectorizer < %s | FileCheck %s
; vectorization requires a vector GEP + extracts, but the cost is offset by being able to efficiently vectorize the rest of the tree
fhahnUnsubmitted
Done
ReplyInline Actions

this is missing an AArch64 triple, you need to add something like -mtriple=arm64-apple-ios.

fhahn: this is missing an AArch64 triple, you need to add something like `-mtriple=arm64-apple-ios`.
fhahnUnsubmitted
Not Done
ReplyInline Actions

Move this directly to the test definition, with a newline between comment and run line

fhahn: Move this directly to the test definition, with a newline between comment and run line
define void @should_vectorize_gep(ptr %base1, ptr %base2, ptr %base_gep) {
fhahnUnsubmitted
Done
ReplyInline Actions

Please add a description to the test. The key issue is that it requires a vector GEP + extracts, but the cost is offset by being able to efficiently vectorize the rest of the tree.

fhahn: Please add a description to the test. The key issue is that it requires a vector GEP + extracts…
; CHECK-LABEL: define void @should_vectorize_gep
fhahnUnsubmitted
Not Done
ReplyInline Actions

More consistent to use base3 instead of base_gep? All bases are used with geps

fhahn: More consistent to use base3 instead of base_gep? All bases are used with geps
; CHECK-SAME: (ptr [[BASE1:%.*]], ptr [[BASE2:%.*]], ptr [[BASE_GEP:%.*]]) {
; CHECK-NEXT: bb:
; CHECK-NEXT: [[LOAD1:%.*]] = load i32, ptr [[BASE1]], align 2
; CHECK-NEXT: [[ZEXT1:%.*]] = zext i32 [[LOAD1]] to i64
; CHECK-NEXT: [[LOAD2:%.*]] = load i32, ptr [[BASE2]], align 2
; CHECK-NEXT: [[ZEXT2:%.*]] = zext i32 [[LOAD2]] to i64
; CHECK-NEXT: [[SUB:%.*]] = sub i64 [[ZEXT1]], [[ZEXT2]]
; CHECK-NEXT: [[GETELEMENTPTR_RES_1:%.*]] = getelementptr i32, ptr [[BASE_GEP]], i64 [[SUB]]
; CHECK-NEXT: [[GETELEMENTPTR1:%.*]] = getelementptr i32, ptr [[BASE1]], i64 1
; CHECK-NEXT: [[GETELEMENTPTR2:%.*]] = getelementptr i32, ptr [[BASE2]], i64 1
; CHECK-NEXT: [[LOAD3:%.*]] = load i32, ptr [[GETELEMENTPTR1]], align 2
; CHECK-NEXT: [[ZEXT3:%.*]] = zext i32 [[LOAD3]] to i64
; CHECK-NEXT: [[LOAD4:%.*]] = load i32, ptr [[GETELEMENTPTR2]], align 2
; CHECK-NEXT: [[ZEXT4:%.*]] = zext i32 [[LOAD4]] to i64
; CHECK-NEXT: [[SUB2:%.*]] = sub i64 [[ZEXT3]], [[ZEXT4]]
; CHECK-NEXT: [[GETELEMENTPTR_RES_2:%.*]] = getelementptr i32, ptr [[BASE_GEP]], i64 [[SUB2]]
; CHECK-NEXT: [[GETELEMENTPTR3:%.*]] = getelementptr i32, ptr [[BASE1]], i64 2
; CHECK-NEXT: [[GETELEMENTPTR4:%.*]] = getelementptr i32, ptr [[BASE2]], i64 2
; CHECK-NEXT: [[LOAD5:%.*]] = load i32, ptr [[GETELEMENTPTR3]], align 2
; CHECK-NEXT: [[ZEXT5:%.*]] = zext i32 [[LOAD5]] to i64
; CHECK-NEXT: [[LOAD6:%.*]] = load i32, ptr [[GETELEMENTPTR4]], align 2
; CHECK-NEXT: [[ZEXT6:%.*]] = zext i32 [[LOAD6]] to i64
; CHECK-NEXT: [[SUB3:%.*]] = sub i64 [[ZEXT5]], [[ZEXT6]]
; CHECK-NEXT: [[GETELEMENTPTR_RES_3:%.*]] = getelementptr i32, ptr [[BASE_GEP]], i64 [[SUB3]]
; CHECK-NEXT: [[GETELEMENTPTR5:%.*]] = getelementptr i32, ptr [[BASE1]], i64 3
; CHECK-NEXT: [[GETELEMENTPTR6:%.*]] = getelementptr i32, ptr [[BASE2]], i64 3
; CHECK-NEXT: [[LOAD7:%.*]] = load i32, ptr [[GETELEMENTPTR5]], align 2
; CHECK-NEXT: [[ZEXT7:%.*]] = zext i32 [[LOAD7]] to i64
; CHECK-NEXT: [[LOAD8:%.*]] = load i32, ptr [[GETELEMENTPTR6]], align 2
; CHECK-NEXT: [[ZEXT8:%.*]] = zext i32 [[LOAD8]] to i64
; CHECK-NEXT: [[SUB4:%.*]] = sub i64 [[ZEXT7]], [[ZEXT8]]
; CHECK-NEXT: [[GETELEMENTPTR_RES_4:%.*]] = getelementptr i32, ptr [[BASE_GEP]], i64 [[SUB4]]
; CHECK-NEXT: call void @use_4(ptr [[GETELEMENTPTR_RES_1]], ptr [[GETELEMENTPTR_RES_2]], ptr [[GETELEMENTPTR_RES_3]], ptr [[GETELEMENTPTR_RES_4]])
; CHECK-NEXT: ret void
;
bb:
%load1 = load i32, ptr %base1, align 2
%zext1 = zext i32 %load1 to i64
fhahnUnsubmitted
Done
ReplyInline Actions

please don't use null in the test. e.g. loading from null like the test does is UB.

fhahn: please don't use `null` in the test. e.g. loading from `null` like the test does is UB.
%load2 = load i32, ptr %base2, align 2
%zext2 = zext i32 %load2 to i64
%sub = sub i64 %zext1, %zext2
%getelementptr.res.1 = getelementptr i32, ptr %base_gep, i64 %sub
%getelementptr1 = getelementptr i32, ptr %base1, i64 1
%getelementptr2 = getelementptr i32, ptr %base2, i64 1
%load3 = load i32, ptr %getelementptr1, align 2
%zext3 = zext i32 %load3 to i64
%load4 = load i32, ptr %getelementptr2, align 2
%zext4= zext i32 %load4 to i64
%sub2 = sub i64 %zext3, %zext4
%getelementptr.res.2 = getelementptr i32, ptr %base_gep, i64 %sub2
%getelementptr3 = getelementptr i32, ptr %base1, i64 2
%getelementptr4 = getelementptr i32, ptr %base2, i64 2
%load5 = load i32, ptr %getelementptr3, align 2
%zext5 = zext i32 %load5 to i64
%load6 = load i32, ptr %getelementptr4, align 2
%zext6 = zext i32 %load6 to i64
%sub3 = sub i64 %zext5, %zext6
%getelementptr.res.3 = getelementptr i32, ptr %base_gep, i64 %sub3
%getelementptr5 = getelementptr i32, ptr %base1, i64 3
%getelementptr6 = getelementptr i32, ptr %base2, i64 3
fhahnUnsubmitted
Not Done
ReplyInline Actions

It would be more compact to use gep instead of get elementary in the names.

fhahn: It would be more compact to use gep instead of get elementary in the names.
%load7 = load i32, ptr %getelementptr5, align 2
%zext7 = zext i32 %load7 to i64
%load8 = load i32, ptr %getelementptr6, align 2
%zext8 = zext i32 %load8 to i64
%sub4 = sub i64 %zext7, %zext8
%getelementptr.res.4 = getelementptr i32, ptr %base_gep, i64 %sub4
call void @use_4(ptr %getelementptr.res.1, ptr %getelementptr.res.2, ptr %getelementptr.res.3, ptr %getelementptr.res.4)
ret void
}
declare void @use_4(ptr, ptr, ptr, ptr)