[PATCH] D93503: [GVN] Propagate llvm.access.group metadata of loads

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D93503#2483527 <https://reviews.llvm.org/D93503#2483527>, @kawashima-fj wrote:

> In D93503#2479425 <https://reviews.llvm.org/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.



================
Comment at: llvm/test/Transforms/GVN/PRE/load-pre-metadata-accsess-group.ll:31
+!1 = distinct !{}
+!2 = distinct !{!2, !3, !4}
+!3 = !{!"llvm.loop.parallel_accesses", !1}
----------------
Is the `!llvm.loop` metadata needed?


================
Comment at: llvm/test/Transforms/GVN/PRE/load-pre-metadata-accsess-group.ll:33
+!3 = !{!"llvm.loop.parallel_accesses", !1}
+!4 = !{!"llvm.loop.vectorize.enable", i1 true}
----------------
That should not be needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93503/new/

https://reviews.llvm.org/D93503