This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Preserve llvm.mem.parallel_loop_access metadata when replacing memcpy with ld/st
ClosedPublic

Authored by dorit on Aug 15 2016, 4:31 AM.

Details

Summary

When InstCombine replaces a memcpy with loads+stores it does not copy over the llvm.mem.parallel_loop_access from the memcpy instruction.

BTW, currently our memcpy's don't get annotated with llvm.mem.parallel_loop_access by clang (just opened a PR for this one: 28980). So this scenario is not yet reproducible automatically (need to add the llvm.mem.parallel_loop_access manually to the memcpy in the .ll).

Diff Detail

Repository
rL LLVM

Event Timeline

dorit updated this revision to Diff 68012.Aug 15 2016, 4:31 AM
dorit retitled this revision from to [InstCombine] Preserve llvm.mem.parallel_loop_access metadata when replacing memcpy with ld/st.
dorit updated this object.
dorit added reviewers: majnemer, reames.
dorit added subscribers: Ayal, gilr, magabari, llvm-commits.
Ayal added inline comments.Aug 16 2016, 4:58 AM
llvm/test/Transforms/InstCombine/mem-par-metadata-memcpy.ll
2 ↗(On Diff #68012)

Can add here that this is related to PR28981, for the broader context.

mkuper added inline comments.Aug 25 2016, 11:26 AM
llvm/test/Transforms/InstCombine/mem-par-metadata-memcpy.ll
13 ↗(On Diff #68012)

Can you move the RUN line to be the first line in the file, before the docs?

15 ↗(On Diff #68012)

You're missing a ":" at the end of your CHECKs and CHECK-NOTs.
But I think if you actually add them, the test will fail, since the CHECK-NOT will match the beginning of the longer line. You'll need to rewrite the pattern to match a full line (adding a {{$}} should work), but even then, I'm not entirely sure this will do the right thing.

mkuper added inline comments.Aug 25 2016, 11:29 AM
llvm/test/Transforms/InstCombine/mem-par-metadata-memcpy.ll
15 ↗(On Diff #68012)

On second thought, the CHECK-NOTs may work fine as-is - although I'm still somewhat suspicious. :-)
In any case, the ":" are still missing.

dorit updated this revision to Diff 69507.Aug 28 2016, 3:13 AM

Addressed Michael's comments about the testcase: moved RUN line to top of file ; added missing ":" after CHECK ; dropped the CHECK-NOTs, since in this specific testcase there will be no additional loads/stores beyond the one's that the positive CHECK catches, so the CHECK-NOTs are redundant.

mkuper accepted this revision.Aug 29 2016, 10:34 AM
mkuper edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 29 2016, 10:34 AM
This revision was automatically updated to reflect the committed changes.