This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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
kawashima-fj requested review of this revision.Dec 17 2020, 5:29 PM

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

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
9

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?

Simplify the test code.

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.

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
fhahn added a comment.Jan 7 2021, 5:39 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?

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
32

Is the !llvm.loop metadata needed?

34

That should not be needed.

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.

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
32

llvm.loop is removed.

34

llvm.loop.vectorize.enable is removed.

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

@fhahn Ping. Could you confirm?

@fhahn Ping. Could you confirm?

@fhahn Ping. I addressed your comment.

fhahn accepted this revision.Mar 30 2021, 7:46 AM

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

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.