Download Raw Diff

Details

Reviewers

ABataev
fhahn
sdesmalen
david-arm

Commits

rG20d0f80dd356: [test] A test case for D146958

Summary

This commit introduces a test for the change introduced by the
[SCEV] Do not plant SCEV checks unnecessarily commit,
D146958.

Diff Detail

Unit TestsFailed

	Time	Test
	50 ms	x64 debian > LLVM.Transforms/LoopVectorize::vector-no-scevcheck.ll

Event Timeline

pawosm01 created this revision.Mar 27 2023, 8:15 AM

Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2023, 8:15 AM

pawosm01 requested review of this revision.Mar 27 2023, 8:15 AM

Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2023, 8:15 AM

Herald added subscribers: llvm-commits, • pcwang-thead. · View Herald Transcript

pawosm01 retitled this revision from [test] A test case for D146839 to [test] A test case for D146958.Mar 27 2023, 8:16 AM

pawosm01 mentioned this in D146958: [SCEV] Do not plant SCEV checks unnecessarily.

fhahn added a subscriber: fhahn.Mar 27 2023, 9:10 AM

fhahn added inline comments.

llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
7	`entry`?
10	Is this load needed or can the value directly be passed as argument?
14	Please shorten the block names and try to make them more descriptive, this will make the test easier to read. E.g. IIUC this could be something like `loop1.header`.
17	Is this branch needed here?
20	Is this load needed or can the value directly be passed as argument?
25	this could be `loop2.header`.
30	is this branch needed?
32	This could be `loop3.header`. Is a 3 level loop nest needed for the test? Would a 2 level loop nest be sufficient?
45	`loop2.latch`?
50	`loop1.latch`
55	`exit`?

Harbormaster completed remote builds in B222030: Diff 508671.Mar 27 2023, 11:09 AM

Failed as expected.

@fhahn Thanks for your comments. If you don't mind, I will address them in the post-review changes of D146958 in coming day(s).

pawosm01 marked an inline comment as done.Mar 28 2023, 6:58 AM

pawosm01 added inline comments.

llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
30	Yes

Test updated as per the reviewer request.

pawosm01 marked 10 inline comments as done.Mar 28 2023, 7:00 AM

pawosm01 added a reviewer: fhahn.

Herald added a subscriber: StephenFan. · View Herald TranscriptMar 28 2023, 7:00 AM

Harbormaster completed remote builds in B222236: Diff 508996.Mar 28 2023, 7:48 AM

seems I had a wrong understanding of the reviewer's intent.

Harbormaster completed remote builds in B223595: Diff 510825.Apr 4 2023, 9:41 AM

pawosm01 edited the summary of this revision. (Show Details)Apr 4 2023, 10:51 AM

Herald added a subscriber: javed.absar. · View Herald TranscriptApr 4 2023, 10:51 AM

pawosm01 added reviewers: sdesmalen, david-arm.Apr 4 2023, 10:52 AM

I'll try to build a relation between D146974 and D146958.

pawosm01 added a child revision: D146958: [SCEV] Do not plant SCEV checks unnecessarily.Apr 4 2023, 11:08 AM

xbolva00 added a subscriber: xbolva00.Apr 4 2023, 11:26 AM

xbolva00 added inline comments.

llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
2	This test needs to pass with current master. Tests are commited as first, so things would be broken.

Harbormaster completed remote builds in B223622: Diff 510861.Apr 4 2023, 11:46 AM

Following reviewer's advice.

wrong diff file picked; this is the correct one.

Looks OK now. @fhahn ?

pawosm01 marked an inline comment as done.Apr 4 2023, 12:13 PM

pawosm01 edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B223635: Diff 510877.Apr 4 2023, 1:00 PM

LGTM! It looks like you've addressed @fhahn's comments.

This revision is now accepted and ready to land.Apr 5 2023, 1:43 AM

fhahn added inline comments.Apr 5 2023, 2:36 AM

llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
4	could you add a brief comment saying what this test is intended to check?
16	Could you add a new function that's basically the same as `@foo` but has `%arrayidx0` in `%loop2.header`?

pawosm01 marked an inline comment as done.Apr 17 2023, 7:18 AM

pawosm01 added inline comments.

llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
16	Could you add a new function that's basically the same as @foo but has %arrayidx0 in %loop2.header? This will not expose the same behavior of the D146958 patch we care about in this test (namely, `vector.scevcheck` will remain).

pawosm01 marked an inline comment as done.Apr 17 2023, 7:27 AM

a short comment added as requested

pawosm01 marked an inline comment as done.Apr 17 2023, 7:30 AM

Harbormaster completed remote builds in B226098: Diff 514226.Apr 17 2023, 9:24 AM

Hi @fhahn, are you happy with the current shape?

fhahn added inline comments.Apr 23 2023, 2:34 PM

llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
16	Right, but that's only with the current version of the patch; it should be relatively straightforward to also handle this case (https://reviews.llvm.org/D146958#inline-1439048). In any case, it won't hurt to have negative test cases as well.

mgabka added a subscriber: mgabka.Apr 24 2023, 2:29 AM

Extended with one more variant of the same test case, as requested.

pawosm01 marked an inline comment as done.Apr 25 2023, 7:03 AM

pawosm01 added inline comments.

llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
16	Hi Florian, your suggestion from the patch review has worked, hence I've modified this test file accordingly. I'd like to have your explicit approval before I merge this one.

LGTM, thanks!

This revision was landed with ongoing or failed builds.Apr 25 2023, 1:37 PM

Closed by commit rG20d0f80dd356: [test] A test case for D146958 (authored by pawosm01). · Explain Why

This revision was automatically updated to reflect the committed changes.

pawosm01 marked an inline comment as done.

pawosm01 added a commit: rG20d0f80dd356: [test] A test case for D146958.

Diff 508671

llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll

This file was added.

				; RUN: opt -passes=loop-vectorize -force-vector-width=2 -force-vector-interleave=1 -S < %s \| FileCheck %s

				xbolva00Unsubmitted Done Reply Inline Actions This test needs to pass with current master. Tests are commited as first, so things would be broken. xbolva00: This test needs to pass with current master. Tests are commited as first, so things would be…
				define void @foo(ptr %pout, ptr %pin, ptr %dims1, ptr %dims2) {
				; CHECK-LABEL: @foo(
				fhahnUnsubmitted Done Reply Inline Actions could you add a brief comment saying what this test is intended to check? fhahn: could you add a brief comment saying what this test is intended to check?
				; CHECK-NOT: vector.scevcheck
				; CHECK: vector.body
				for.cond1.preheader.lr.ph:
				fhahnUnsubmitted Done Reply Inline Actions `entry`? fhahn: `entry`?
				%arrayidx7 = getelementptr inbounds i64, ptr %dims1, i64 1
				%arrayidx11 = getelementptr inbounds i64, ptr %dims2, i64 1
				%0 = load i64, ptr %arrayidx7, align 8
				fhahnUnsubmitted Done Reply Inline Actions Is this load needed or can the value directly be passed as argument? fhahn: Is this load needed or can the value directly be passed as argument?
				%cmp845 = icmp sgt i64 %0, 0
				br label %for.cond6.preheader.lr.ph

				for.cond6.preheader.lr.ph: ; preds = %for.cond.cleanup4, %for.cond1.preheader.lr.ph
				fhahnUnsubmitted Done Reply Inline Actions Please shorten the block names and try to make them more descriptive, this will make the test easier to read. E.g. IIUC this could be something like `loop1.header`. fhahn: Please shorten the block names and try to make them more descriptive, this will make the test…
				%1 = phi i64 [ 0, %for.cond1.preheader.lr.ph ], [ %inc26, %for.cond.cleanup4 ]
				%2 = getelementptr double, ptr %pin, i64 %1
				fhahnUnsubmitted Done Reply Inline Actions Could you add a new function that's basically the same as `@foo` but has `%arrayidx0` in `%loop2.header`? fhahn: Could you add a new function that's basically the same as `@foo` but has `%arrayidx0` in…
				pawosm01AuthorUnsubmitted Done Reply Inline Actions Could you add a new function that's basically the same as @foo but has %arrayidx0 in %loop2.header? This will not expose the same behavior of the D146958 patch we care about in this test (namely, `vector.scevcheck` will remain). pawosm01: > Could you add a new function that's basically the same as @foo but has %arrayidx0 in %loop2.
				fhahnUnsubmitted Done Reply Inline Actions Right, but that's only with the current version of the patch; it should be relatively straightforward to also handle this case (https://reviews.llvm.org/D146958#inline-1439048). In any case, it won't hurt to have negative test cases as well. fhahn: Right, but that's only with the current version of the patch; it should be relatively…
				pawosm01AuthorUnsubmitted Done Reply Inline Actions Hi Florian, your suggestion from the patch review has worked, hence I've modified this test file accordingly. I'd like to have your explicit approval before I merge this one. pawosm01: Hi Florian, your suggestion from the patch review has worked, hence I've modified this test…
				br i1 %cmp845, label %for.cond6.preheader.us.preheader, label %for.cond.cleanup4
				fhahnUnsubmitted Done Reply Inline Actions Is this branch needed here? fhahn: Is this branch needed here?

				for.cond6.preheader.us.preheader: ; preds = %for.cond6.preheader.lr.ph
				%.pre = load i64, ptr %arrayidx11, align 8
				fhahnUnsubmitted Done Reply Inline Actions Is this load needed or can the value directly be passed as argument? fhahn: Is this load needed or can the value directly be passed as argument?
				%mul14.us = mul nsw i64 %1, %.pre
				%3 = getelementptr double, ptr %pin, i64 %mul14.us
				br label %for.cond6.preheader.us

				for.cond6.preheader.us: ; preds = %for.cond6.for.cond.cleanup9_crit_edge.us, %for.cond6.preheader.us.preheader
				fhahnUnsubmitted Done Reply Inline Actions this could be `loop2.header`. fhahn: this could be `loop2.header`.
				%i.048.us = phi i64 [ %inc23.us, %for.cond6.for.cond.cleanup9_crit_edge.us ], [ 0, %for.cond6.preheader.us.preheader ]
				%mul.us = mul nsw i64 %i.048.us, %.pre
				%arrayidx12.us = getelementptr inbounds double, ptr %2, i64 %mul.us
				%mul19.us = mul nsw i64 %i.048.us, %0
				br label %for.body10.us
				fhahnUnsubmitted Done Reply Inline Actions is this branch needed? fhahn: is this branch needed?
				pawosm01AuthorUnsubmitted Done Reply Inline Actions Yes pawosm01: Yes

				for.body10.us: ; preds = %for.cond6.preheader.us, %for.body10.us
				fhahnUnsubmitted Done Reply Inline Actions This could be `loop3.header`. Is a 3 level loop nest needed for the test? Would a 2 level loop nest be sufficient? fhahn: This could be `loop3.header`. Is a 3 level loop nest needed for the test? Would a 2 level loop…
				%j.046.us = phi i64 [ 0, %for.cond6.preheader.us ], [ %inc.us, %for.body10.us ]
				%4 = load double, ptr %arrayidx12.us, align 8
				%arrayidx16.us = getelementptr inbounds double, ptr %3, i64 %j.046.us
				%5 = load double, ptr %arrayidx16.us, align 8
				%add17.us = fadd contract double %4, %5
				%6 = getelementptr double, ptr %pout, i64 %mul19.us
				%arrayidx21.us = getelementptr inbounds double, ptr %6, i64 %j.046.us
				store double %add17.us, ptr %arrayidx21.us, align 8
				%inc.us = add nuw nsw i64 %j.046.us, 1
				%cmp8.us = icmp slt i64 %inc.us, %0
				br i1 %cmp8.us, label %for.body10.us, label %for.cond6.for.cond.cleanup9_crit_edge.us

				for.cond6.for.cond.cleanup9_crit_edge.us: ; preds = %for.body10.us
				fhahnUnsubmitted Done Reply Inline Actions `loop2.latch`? fhahn: `loop2.latch`?
				%inc23.us = add nuw nsw i64 %i.048.us, 1
				%exitcond = icmp eq i64 %inc23.us, %0
				br i1 %exitcond, label %for.cond.cleanup4, label %for.cond6.preheader.us

				for.cond.cleanup4: ; preds = %for.cond6.for.cond.cleanup9_crit_edge.us, %for.cond6.preheader.lr.ph
				fhahnUnsubmitted Done Reply Inline Actions `loop1.latch` fhahn: `loop1.latch`
				%inc26 = add nuw nsw i64 %1, 1
				%cmp = icmp slt i64 %inc26, %0
				br i1 %cmp, label %for.cond6.preheader.lr.ph, label %for.cond.cleanup

				for.cond.cleanup: ; preds = %for.cond.cleanup4
				fhahnUnsubmitted Done Reply Inline Actions `exit`? fhahn: `exit`?
				ret void
				}

This is an archive of the discontinued LLVM Phabricator instance.

[test] A test case for D146958
ClosedPublic

Details

Diff Detail

Unit TestsFailed

Event Timeline

Revision Contents

Diff 508671

llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll

This is an archive of the discontinued LLVM Phabricator instance.

[test] A test case for D146958ClosedPublic

Details

Diff Detail

Unit TestsFailed

Event Timeline

Revision Contents

Diff 508671

llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll

[test] A test case for D146958
ClosedPublic