This is an archive of the discontinued LLVM Phabricator instance.

Differential D93503

[GVN] Propagate llvm.access.group metadata of loads
ClosedPublic

Authored by kawashima-fj on Dec 17 2020, 5:29 PM.

Details

Reviewers
fhahn
asbirlea
skatkov
Commits
rG5fac7c60467c: [GVN] Propagate llvm.access.group metadata of loads
Summary

Before this change, the llvm.access.group metadata was dropped
when moving a load instruction in GVN. This prevents vectorizing
a C/C++ loop with #pragma clang loop vectorize(assume_safety).
This change propagates the metadata as well as other metadata if
it is safe (the move-destination basic block and source basic
block belong to the same loop).

Diff Detail

Event Timeline

kawashima-fj created this revision.Dec 17 2020, 5:29 PM
Herald added a subscriber: hiraditya. · View Herald TranscriptDec 17 2020, 5:29 PM
kawashima-fj requested review of this revision.Dec 17 2020, 5:29 PM
Herald added a subscriber: llvm-commits. · View Herald TranscriptDec 17 2020, 5:29 PM
Harbormaster completed remote builds in B82889: Diff 312650.Dec 17 2020, 6:16 PM
kawashima-fj added a comment.Dec 25 2020, 12:36 AM

Ping.
I know many people are on holiday this week. If you have time, please review this patch. I'm on holiday from tomorrow.

lebedev.ri added a reviewer: skatkov.Dec 25 2020, 12:45 AM
kawashima-fj added a comment.Jan 5 2021, 4:11 AM

Ping.
Can anyone review? Is there an appropriate reviewer?

fhahn added a comment.Jan 5 2021, 7:06 AM

This seems reasonable to me, as we should only reduce the number of loads executed during PRE. If they are in the same loop, the metadata should stay valid, as we do not access any additional locations.

Would it be possible to add a simple test case where we move a load from a sub-loop into a parent loop?

llvm/test/Transforms/GVN/PRE/load-pre-metadata-accsess-group.ll
8

can you simplify the test to only the required instructions? e.g. not all access here should be needed, nor all parameters, nor all arithmetic instructions, etc

Once it is cut down, it would probably be good to explicitly match the blocks that were inserted as part of GVN PRE.

Also, do we need the dedicated preheader/exit blocks?

kawashima-fj updated this revision to Diff 315029.Jan 6 2021, 5:38 PM

Simplify the test code.

kawashima-fj added a comment.Jan 6 2021, 5:50 PM
In D93503#2479425, @fhahn wrote:

This seems reasonable to me, as we should only reduce the number of loads executed during PRE. If they are in the same loop, the metadata should stay valid, as we do not access any additional locations.

Would it be possible to add a simple test case where we move a load from a sub-loop into a parent loop?

Thanks for your review.
The test in the previous revision is refined. I might not understand your comment correctly. If I missed something, please let me know.
My colleague, who fixed this issue, is busy now. The requested additional test can be added next week or so.

asbirlea added a comment.Jan 6 2021, 5:54 PM

Looks reasonable to me as well, that the metadata info should hold at the same loop level.

kawashima-fj marked an inline comment as done.Jan 6 2021, 5:54 PM
Harbormaster completed remote builds in B84279: Diff 315029.Jan 6 2021, 6:32 PM
fhahn added a comment.Jan 7 2021, 5:39 AM
In D93503#2483527, @kawashima-fj wrote:
In D93503#2479425, @fhahn wrote:

This seems reasonable to me, as we should only reduce the number of loads executed during PRE. If they are in the same loop, the metadata should stay valid, as we do not access any additional locations.

Would it be possible to add a simple test case where we move a load from a sub-loop into a parent loop?

Thanks for your review.
The test in the previous revision is refined. I might not understand your comment correctly. If I missed something, please let me know.

Thanks for cleaning up the test. It left a few additional small suggestions. I think for that test it would be good to add CHECK lines for all code generated during PRE. Given it is quite small, you could use llvm/utils/update_test_checks.py to auto-generate the check lines. Also, I think it would be good to pre-commit the test with check lines for current trunk and then update the patch to just show the diff in CHECK lines with this patch.

My colleague, who fixed this issue, is busy now. The requested additional test can be added next week or so.

That would be great.

llvm/test/Transforms/GVN/PRE/load-pre-metadata-accsess-group.ll
31

Is the !llvm.loop metadata needed?

33

That should not be needed.

kawashima-fj updated this revision to Diff 316840.Jan 14 2021, 11:55 PM

Update test.

  • Remove unnecessary metadata (!llvm.loop, llvm.loop.parallel_accesses, llvm.loop.vectorize.enable)
  • Add a test (function) which moves a load from a sub-loop into a parent loop
  • Use llvm/utils/update_test_checks.py to generate CHECK lines
kawashima-fj marked 2 inline comments as done.Jan 15 2021, 12:00 AM

I'm sorry for the late response. I (and my colleague) updated the test.

In D93503#2484226, @fhahn wrote:

Thanks for cleaning up the test. It left a few additional small suggestions. I think for that test it would be good to add CHECK lines for all code generated during PRE. Given it is quite small, you could use llvm/utils/update_test_checks.py to auto-generate the check lines. Also, I think it would be good to pre-commit the test with check lines for current trunk and then update the patch to just show the diff in CHECK lines with this patch.

You mean I should push two commits to the GitHub repository, right?
The first one is only the test file with CHECK-lines generated by update_test_checks.py with current master branch.
The second one is update of the GVN source file and CHECK-lines generated by update_test_checks.py with the new source code.
I'm newbie to LLVM. Your suggestion is very helpful. Thanks.

llvm/test/Transforms/GVN/PRE/load-pre-metadata-accsess-group.ll
31

llvm.loop is removed.

33

llvm.loop.vectorize.enable is removed.

Harbormaster completed remote builds in B85303: Diff 316840.Jan 15 2021, 12:29 AM
kawashima-fj marked 2 inline comments as done.Jan 22 2021, 7:29 PM

Ping.
@fhahn @asbirlea Is this OK?

asbirlea accepted this revision.Jan 22 2021, 11:53 PM

LGTM.
Please wait to see if @fhahn has additional feedback.

This revision is now accepted and ready to land.Jan 22 2021, 11:53 PM
kawashima-fj added a comment.Feb 1 2021, 4:19 AM

@fhahn Ping. Could you confirm?

kawashima-fj added a comment.Feb 18 2021, 6:54 AM

@fhahn Ping. Could you confirm?

kawashima-fj added a comment.Mar 30 2021, 7:43 AM

@fhahn Ping. I addressed your comment.

fhahn accepted this revision.Mar 30 2021, 7:46 AM
In D93503#2500308, @kawashima-fj wrote:

I'm sorry for the late response. I (and my colleague) updated the test.

In D93503#2484226, @fhahn wrote:

Thanks for cleaning up the test. It left a few additional small suggestions. I think for that test it would be good to add CHECK lines for all code generated during PRE. Given it is quite small, you could use llvm/utils/update_test_checks.py to auto-generate the check lines. Also, I think it would be good to pre-commit the test with check lines for current trunk and then update the patch to just show the diff in CHECK lines with this patch.

You mean I should push two commits to the GitHub repository, right?
The first one is only the test file with CHECK-lines generated by update_test_checks.py with current master branch.
The second one is update of the GVN source file and CHECK-lines generated by update_test_checks.py with the new source code.

Yep that would be the idea. You can just push the update to the existing test and then update the review.

The change LGTM.

Closed by commit rG5fac7c60467c: [GVN] Propagate llvm.access.group metadata of loads (authored by kawashima-fj). · Explain WhyMar 31 2021, 6:01 PM
This revision was automatically updated to reflect the committed changes.
kawashima-fj added a commit: rG5fac7c60467c: [GVN] Propagate llvm.access.group metadata of loads.
kawashima-fj mentioned this in rG565b28f57b55: [GVN][NFC] Pre-commit test for D93503.

Revision Contents

PathSize
llvm/
lib/
Transforms/
Scalar/
4 lines
test/
Transforms/
GVN/
PRE/
4 lines

Diff 334571

llvm/lib/Transforms/Scalar/GVN.cpp

Show First 20 Lines • Show All 1,400 Lines • ▼ Show 20 Lines if (Tags)
NewLoad->setAAMetadata(Tags); NewLoad->setAAMetadata(Tags);
if (auto *MD = LI->getMetadata(LLVMContext::MD_invariant_load)) if (auto *MD = LI->getMetadata(LLVMContext::MD_invariant_load))
NewLoad->setMetadata(LLVMContext::MD_invariant_load, MD); NewLoad->setMetadata(LLVMContext::MD_invariant_load, MD);
if (auto *InvGroupMD = LI->getMetadata(LLVMContext::MD_invariant_group)) if (auto *InvGroupMD = LI->getMetadata(LLVMContext::MD_invariant_group))
NewLoad->setMetadata(LLVMContext::MD_invariant_group, InvGroupMD); NewLoad->setMetadata(LLVMContext::MD_invariant_group, InvGroupMD);
if (auto *RangeMD = LI->getMetadata(LLVMContext::MD_range)) if (auto *RangeMD = LI->getMetadata(LLVMContext::MD_range))
NewLoad->setMetadata(LLVMContext::MD_range, RangeMD); NewLoad->setMetadata(LLVMContext::MD_range, RangeMD);
if (auto *AccessMD = LI->getMetadata(LLVMContext::MD_access_group))
if (this->LI && this->LI->getLoopFor(LI->getParent()) ==
this->LI->getLoopFor(UnavailablePred))
NewLoad->setMetadata(LLVMContext::MD_access_group, AccessMD);
// We do not propagate the old load's debug location, because the new // We do not propagate the old load's debug location, because the new
// load now lives in a different BB, and we want to avoid a jumpy line // load now lives in a different BB, and we want to avoid a jumpy line
// table. // table.
// FIXME: How do we retain source locations without causing poor debugging // FIXME: How do we retain source locations without causing poor debugging
// behavior? // behavior?
// Add the newly created load. // Add the newly created load.
▲ Show 20 Lines • Show All 1,542 Lines • Show Last 20 Lines

llvm/test/Transforms/GVN/PRE/load-pre-metadata-accsess-group.ll

; NOTE: Assertions have been autogenerated by utils/update_test_checks.py ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt < %s -loops -basic-aa -gvn -enable-load-pre -S | FileCheck %s ; RUN: opt < %s -loops -basic-aa -gvn -enable-load-pre -S | FileCheck %s
define dso_local void @test1(i32* nocapture readonly %aa, i32* nocapture %bb) local_unnamed_addr { define dso_local void @test1(i32* nocapture readonly %aa, i32* nocapture %bb) local_unnamed_addr {
; CHECK-LABEL: @test1( ; CHECK-LABEL: @test1(
; CHECK-NEXT: entry: ; CHECK-NEXT: entry:
; CHECK-NEXT: [[IDX:%.*]] = getelementptr inbounds i32, i32* [[BB:%.*]], i64 1 ; CHECK-NEXT: [[IDX:%.*]] = getelementptr inbounds i32, i32* [[BB:%.*]], i64 1
; CHECK-NEXT: [[IDX2:%.*]] = getelementptr inbounds i32, i32* [[AA:%.*]], i64 1 ; CHECK-NEXT: [[IDX2:%.*]] = getelementptr inbounds i32, i32* [[AA:%.*]], i64 1
fhahnUnsubmitted
Done
ReplyInline Actions

can you simplify the test to only the required instructions? e.g. not all access here should be needed, nor all parameters, nor all arithmetic instructions, etc

Once it is cut down, it would probably be good to explicitly match the blocks that were inserted as part of GVN PRE.

Also, do we need the dedicated preheader/exit blocks?

fhahn: can you simplify the test to only the required instructions? e.g. not all access here should be…
; CHECK-NEXT: [[TMP0:%.*]] = load i32, i32* [[IDX2]], align 4 ; CHECK-NEXT: [[TMP0:%.*]] = load i32, i32* [[IDX2]], align 4
; CHECK-NEXT: store i32 [[TMP0]], i32* [[IDX]], align 4 ; CHECK-NEXT: store i32 [[TMP0]], i32* [[IDX]], align 4
; CHECK-NEXT: br label [[FOR_BODY:%.*]] ; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body: ; CHECK: for.body:
; CHECK-NEXT: [[TMP1:%.*]] = phi i32 [ [[TMP0]], [[ENTRY:%.*]] ], [ [[DOTPRE:%.*]], [[FOR_BODY_FOR_BODY_CRIT_EDGE:%.*]] ] ; CHECK-NEXT: [[TMP1:%.*]] = phi i32 [ [[TMP0]], [[ENTRY:%.*]] ], [ [[DOTPRE:%.*]], [[FOR_BODY_FOR_BODY_CRIT_EDGE:%.*]] ]
; CHECK-NEXT: [[INDVARS_IV:%.*]] = phi i64 [ 0, [[ENTRY]] ], [ [[INDVARS_IV_NEXT:%.*]], [[FOR_BODY_FOR_BODY_CRIT_EDGE]] ] ; CHECK-NEXT: [[INDVARS_IV:%.*]] = phi i64 [ 0, [[ENTRY]] ], [ [[INDVARS_IV_NEXT:%.*]], [[FOR_BODY_FOR_BODY_CRIT_EDGE]] ]
; CHECK-NEXT: [[IDX4:%.*]] = getelementptr inbounds i32, i32* [[AA]], i64 [[INDVARS_IV]] ; CHECK-NEXT: [[IDX4:%.*]] = getelementptr inbounds i32, i32* [[AA]], i64 [[INDVARS_IV]]
; CHECK-NEXT: [[TMP2:%.*]] = load i32, i32* [[IDX4]], align 4, !llvm.access.group !0 ; CHECK-NEXT: [[TMP2:%.*]] = load i32, i32* [[IDX4]], align 4, !llvm.access.group !0
; CHECK-NEXT: [[MUL:%.*]] = mul nsw i32 [[TMP1]], [[TMP2]] ; CHECK-NEXT: [[MUL:%.*]] = mul nsw i32 [[TMP1]], [[TMP2]]
; CHECK-NEXT: store i32 [[MUL]], i32* [[IDX4]], align 4, !llvm.access.group !0 ; CHECK-NEXT: store i32 [[MUL]], i32* [[IDX4]], align 4, !llvm.access.group !0
; CHECK-NEXT: [[INDVARS_IV_NEXT]] = add nuw nsw i64 [[INDVARS_IV]], 1 ; CHECK-NEXT: [[INDVARS_IV_NEXT]] = add nuw nsw i64 [[INDVARS_IV]], 1
; CHECK-NEXT: [[EXITCOND:%.*]] = icmp ne i64 [[INDVARS_IV_NEXT]], 100 ; CHECK-NEXT: [[EXITCOND:%.*]] = icmp ne i64 [[INDVARS_IV_NEXT]], 100
; CHECK-NEXT: br i1 [[EXITCOND]], label [[FOR_BODY_FOR_BODY_CRIT_EDGE]], label [[FOR_END:%.*]] ; CHECK-NEXT: br i1 [[EXITCOND]], label [[FOR_BODY_FOR_BODY_CRIT_EDGE]], label [[FOR_END:%.*]]
; CHECK: for.body.for.body_crit_edge: ; CHECK: for.body.for.body_crit_edge:
; CHECK-NEXT: [[DOTPRE]] = load i32, i32* [[IDX]], align 4 ; CHECK-NEXT: [[DOTPRE]] = load i32, i32* [[IDX]], align 4, !llvm.access.group !0
; CHECK-NEXT: br label [[FOR_BODY]] ; CHECK-NEXT: br label [[FOR_BODY]]
; CHECK: for.end: ; CHECK: for.end:
; CHECK-NEXT: ret void ; CHECK-NEXT: ret void
; ;
entry: entry:
%idx = getelementptr inbounds i32, i32* %bb, i64 1 %idx = getelementptr inbounds i32, i32* %bb, i64 1
%idx2 = getelementptr inbounds i32, i32* %aa, i64 1 %idx2 = getelementptr inbounds i32, i32* %aa, i64 1
%0 = load i32, i32* %idx2, align 4 %0 = load i32, i32* %idx2, align 4
fhahnUnsubmitted
Done
ReplyInline Actions

Is the !llvm.loop metadata needed?

fhahn: Is the `!llvm.loop` metadata needed?
kawashima-fjAuthorUnsubmitted
Done
ReplyInline Actions

llvm.loop is removed.

kawashima-fj: `llvm.loop` is removed.
store i32 %0, i32* %idx, align 4 store i32 %0, i32* %idx, align 4
br label %for.body br label %for.body
fhahnUnsubmitted
Done
ReplyInline Actions

That should not be needed.

fhahn: That should not be needed.
kawashima-fjAuthorUnsubmitted
Done
ReplyInline Actions

llvm.loop.vectorize.enable is removed.

kawashima-fj: `llvm.loop.vectorize.enable` is removed.
for.body: for.body:
%indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ] %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
%idx4 = getelementptr inbounds i32, i32* %aa, i64 %indvars.iv %idx4 = getelementptr inbounds i32, i32* %aa, i64 %indvars.iv
%1 = load i32, i32* %idx4, align 4, !llvm.access.group !0 %1 = load i32, i32* %idx4, align 4, !llvm.access.group !0
%2 = load i32, i32* %idx, align 4, !llvm.access.group !0 %2 = load i32, i32* %idx, align 4, !llvm.access.group !0
%mul = mul nsw i32 %2, %1 %mul = mul nsw i32 %2, %1
store i32 %mul, i32* %idx4, align 4, !llvm.access.group !0 store i32 %mul, i32* %idx4, align 4, !llvm.access.group !0
Show All 21 Lines
; CHECK: for.body2: ; CHECK: for.body2:
; CHECK-NEXT: [[TMP1:%.*]] = phi i32 [ [[TMP0]], [[FOR_BODY]] ], [ [[DOTPRE1:%.*]], [[FOR_BODY2_FOR_BODY2_CRIT_EDGE:%.*]] ] ; CHECK-NEXT: [[TMP1:%.*]] = phi i32 [ [[TMP0]], [[FOR_BODY]] ], [ [[DOTPRE1:%.*]], [[FOR_BODY2_FOR_BODY2_CRIT_EDGE:%.*]] ]
; CHECK-NEXT: [[TMP2:%.*]] = phi i32 [ [[DOTPRE]], [[FOR_BODY]] ], [ [[MUL:%.*]], [[FOR_BODY2_FOR_BODY2_CRIT_EDGE]] ] ; CHECK-NEXT: [[TMP2:%.*]] = phi i32 [ [[DOTPRE]], [[FOR_BODY]] ], [ [[MUL:%.*]], [[FOR_BODY2_FOR_BODY2_CRIT_EDGE]] ]
; CHECK-NEXT: [[INDVARS2_IV:%.*]] = phi i64 [ 0, [[FOR_BODY]] ], [ 1, [[FOR_BODY2_FOR_BODY2_CRIT_EDGE]] ] ; CHECK-NEXT: [[INDVARS2_IV:%.*]] = phi i64 [ 0, [[FOR_BODY]] ], [ 1, [[FOR_BODY2_FOR_BODY2_CRIT_EDGE]] ]
; CHECK-NEXT: [[MUL]] = mul nsw i32 [[TMP1]], [[TMP2]] ; CHECK-NEXT: [[MUL]] = mul nsw i32 [[TMP1]], [[TMP2]]
; CHECK-NEXT: store i32 [[MUL]], i32* [[AA]], align 4, !llvm.access.group !1 ; CHECK-NEXT: store i32 [[MUL]], i32* [[AA]], align 4, !llvm.access.group !1
; CHECK-NEXT: br i1 true, label [[FOR_BODY2_FOR_BODY2_CRIT_EDGE]], label [[FOR_END:%.*]] ; CHECK-NEXT: br i1 true, label [[FOR_BODY2_FOR_BODY2_CRIT_EDGE]], label [[FOR_END:%.*]]
; CHECK: for.body2.for.body2_crit_edge: ; CHECK: for.body2.for.body2_crit_edge:
; CHECK-NEXT: [[DOTPRE1]] = load i32, i32* [[IDX]], align 4 ; CHECK-NEXT: [[DOTPRE1]] = load i32, i32* [[IDX]], align 4, !llvm.access.group !1
; CHECK-NEXT: br label [[FOR_BODY2]] ; CHECK-NEXT: br label [[FOR_BODY2]]
; CHECK: for.end: ; CHECK: for.end:
; CHECK-NEXT: br i1 false, label [[FOR_END_FOR_BODY_CRIT_EDGE:%.*]], label [[END:%.*]] ; CHECK-NEXT: br i1 false, label [[FOR_END_FOR_BODY_CRIT_EDGE:%.*]], label [[END:%.*]]
; CHECK: for.end.for.body_crit_edge: ; CHECK: for.end.for.body_crit_edge:
; CHECK-NEXT: br label [[FOR_BODY]] ; CHECK-NEXT: br label [[FOR_BODY]]
; CHECK: end: ; CHECK: end:
; CHECK-NEXT: ret void ; CHECK-NEXT: ret void
; ;
Show All 32 Lines