This is an archive of the discontinued LLVM Phabricator instance.

[Polly] [DependenceInfo] change WAR generation, Read will not block Read
ClosedPublic

Authored by bollu on Apr 13 2017, 7:21 AM.

Details

Summary

Earlier, the call to buildFlow was:

WAR = buildFlow(Write, Read, MustWrite, Schedule).

This meant that Read could block another Read, since must-sources can
block each other.

Fixed the call to buildFlow to correctly compute Read. The resulting
code needs to do some ISL juggling to get the output we want.

Bug report: https://bugs.llvm.org/show_bug.cgi?id=32623

Event Timeline

bollu created this revision.Apr 13 2017, 7:21 AM
bollu added a comment.Apr 15 2017, 6:49 AM

@jdoerfert , @Meinersbur : ping, review please?

bollu updated this revision to Diff 95753.Apr 19 2017, 8:16 AM
  • separate out WAR generation into a separate pure function
bollu added a comment.Apr 19 2017, 8:18 AM

@grosser, @Meinersbur, @jdoerfert, @gareevroman : This change affects the dependence generation, so I'd like someone else to take a look at it before I check it in.

Meinersbur accepted this revision.Apr 20 2017, 5:04 AM
Meinersbur added inline comments.
lib/Analysis/DependenceInfo.cpp
370–371

Functions usually have a Doxygen comment (with triple slashes, brief description and @param/@return annotation)

372–402

This can be done way simpler. Suggestions:

  1. Negate all coefficients of the Schedule (e.g. S[i] -> [1, i] becomes S[i] -> [-1,-i] ) and compute normal RAW dependencies on that. Order of reads and write are reversed, so you get WAR dependencies.
  1. You only want dependencies of the form [Write -> Read], but you have [Write -> Read+MustWrite] (after you get rid of the MemAccess part). Just intersect with Read's domain.
return isl_union_map_intersect_domain(isl_union_map_domain_factor_domain(WAROverestimated), isl_union_map_domain(Read));
  1. Iterate over all maps in the union (foreachElt) and filter out all map spaces that are not of the form [Write -> Read].

Suggestion 2+3 actually require accesses that have access-level tags (Dependences::AL_Access) to identifiy whether it is a read or a write, so it might not really be simpler.

My favorite is suggestion 1. It it easiest to understand (even the may-write part becomes obvious) and has the least computational overhead, apart from copying an entire Schedule once.

This version should be correct as well, however.

376

I like the annotation, I do these as well. Can you make the style more consistent with isl's string output?

  • Always surrounded by braces
  • Nested tuples surrounded by brackets (not parentheses)

Also MemoryAccess have Ids of the form Stmt_S0_Read0_MemRef_C. MemRef_C alone is an Id of a ScopArrayInfo. I am using Element[] for this purpose.

This revision is now accepted and ready to land.Apr 20 2017, 5:04 AM

@Meinersbur: Would you be OK with me pushing this patch, and changing the generation to the simpler method proposed in the next patch? I would like to get correctness first.

Yes, this is why I accepted the patch.

This revision was automatically updated to reflect the committed changes.