This is an archive of the discontinued LLVM Phabricator instance.

[SROA] Preserve llvm.mem.parallel_loop_access metadata
ClosedPublic

Authored by dorit on Aug 16 2016, 5:32 AM.

Details

Summary

SROA doesn't preserve the llvm.mem.parallel_loop_access metadata when it transforms loads/stores.

(This patch is not a systematic effort to catch all the cases missed by SROA, but it at least covers those exposed by this testcase / by PR28981).

Diff Detail

Repository
rL LLVM

Event Timeline

dorit updated this revision to Diff 68028.Aug 16 2016, 5:32 AM
dorit retitled this revision from to [SROA] Preserve llvm.mem.parallel_loop_access metadata.
dorit updated this object.
dorit added a reviewer: reames.
dorit added subscribers: Ayal, gilr, magabari and 2 others.
hfinkel edited edge metadata.Aug 27 2016, 2:32 PM

SROA doesn't preserve the llvm.mem.parallel_loop_access metadata when it transforms loads/stores.
(This patch is not a systematic effort to catch all the cases missed by SROA, but it at least covers those exposed by this testcase / by PR28981).

The higher-level questions here clearly seem to be:

  1. Does SROA every do anything that will add loop-carried dependencies? If not, we could do this everywhere.
  2. Does SROA also drop AA metadata we should be preserving in the same circumstances? I only see metadata handling in speculatePHINodeLoads and speculateSelectInstLoads, but nowhere else.
chandlerc edited edge metadata.Aug 27 2016, 4:09 PM

SROA doesn't preserve the llvm.mem.parallel_loop_access metadata when it transforms loads/stores.
(This patch is not a systematic effort to catch all the cases missed by SROA, but it at least covers those exposed by this testcase / by PR28981).

The higher-level questions here clearly seem to be:

  1. Does SROA every do anything that will add loop-carried dependencies? If not, we could do this everywhere.

Off the top of my head I can't figure out how it could do this anywhere other than the PHI speculation parts.

  1. Does SROA also drop AA metadata we should be preserving in the same circumstances? I only see metadata handling in speculatePHINodeLoads and speculateSelectInstLoads, but nowhere else.

I can absolutely believe we're dropping metadata when we rewrite stuff but fail to promote it to registers. I think the rewriting logic here never got the most careful of analysis because we "usually" think of it as rewriting in preparation for promotion, but we definitely have lots of rewrites at this point that don't involve in promotion so we should probably systematically go through this.

dorit updated this revision to Diff 69510.Aug 28 2016, 3:45 AM
dorit edited edge metadata.

Slightly updated the testcase: Moved the RUN command to the top of the file; added CHECK and CHECK-NOTs to verify not only that stores without metadata don't appear before the first positive match, but that they also don't appear between and after the positive matches.

dorit added a comment.Aug 28 2016, 3:57 AM

...

I can absolutely believe we're dropping metadata when we rewrite stuff but fail to promote it to registers. I think the rewriting logic here never got the most careful of analysis because we "usually" think of it as rewriting in preparation for promotion, but we definitely have lots of rewrites at this point that don't involve in promotion so we should probably systematically go through this.

Agreed... Is it ok though to commit this patch, as a tiny step towards that (which allows the mem.parallel tags reach the vectorizer) ?

delena added inline comments.Sep 19 2016, 1:19 AM
llvm/lib/Transforms/Scalar/SROA.cpp
2556 ↗(On Diff #69510)

Please use copyMetadata() instead.

dorit updated this revision to Diff 71785.Sep 19 2016, 2:45 AM

Replaced get/setMetadata with copyMetadata as per Elena's comment.

delena accepted this revision.Sep 19 2016, 2:48 AM
delena added a reviewer: delena.
This revision is now accepted and ready to land.Sep 19 2016, 2:48 AM
This revision was automatically updated to reflect the committed changes.