[PATCH] [Polly][Refactor][NFC] Simplify and clean the handling of (new) access relations

[Johannes Doerfert]

On 10/10, Tobias Grosser wrote:
> On 10.10.2014 16:24, Johannes Doerfert wrote:
> >================
> >Comment at: include/polly/ScopInfo.h:245
> >@@ +244,3 @@
> >+  /// @brief Get the new access function imported or set by a pass
> >+  isl_map *getNewAccessRelation() const;
> >+
> >----------------
> >grosser wrote:
> >>jdoerfert wrote:
> >>>dpeixott wrote:
> >>>>Does these two methods need to be public? If not it would be good to make them private to avoid confusion over which method to call (e.g. `getAccessRelation`, `getNewAccessRelation`, `getNewestAccessRelation`). I'm not too familiar with the API, so do whatever makes sense.
> >>>>
> >>>>Otherwise, LGTM.
> >>>I will expose getNewestAccessRelation and replace all uses (only in 2 files outside the ScopInfo itself, at the moment).
> >>>
> >>>I cannot see any reason not to use the newest access function except in the dependence analysi but the importer runs after that pass and nobody else (at least for now) uses the functionality of a new access relation anyway.
> >>>
> >>>Thanks!
> >>I agree the three function names are rather confusing. What about just having getOriginalAccessRelation() and getAccessRelation(), with the last implementing what getNewestAccessRelation currently does?
> >>
> >>In general all places in Polly should use the newest access relation which becomes the normal access relation. So even the introduction of getNewAccessRelation was probably a bad idea.
> >I will do that.
> 
> Cool.
> 
> >================
> >Comment at: lib/Analysis/ScopInfo.cpp:410
> >@@ +409,3 @@
> >+  return isl_pw_multi_aff_from_map(ScheduledAccRel);
> >+}
> >+
> >----------------
> >grosser wrote:
> >>I am not sure if pulling this into the MemoryAccess is an improvement. This is a rather complicated sequence of operations that is rather specific to the subsequent use in isl_ast_build_access_from_pw_multi_aff(). As getNewAccessOperand() is rather short and we use this sequence only once, I would suggest to leave the code as it is.
> >Can I leave it here if my next patch uses it? [When I store back the aggregated privatized value I need to access the memory location in the new schedule].
> 
> Sure. Could you just note this in the commit message, that people don't get
> surprised why you pull it out.
Ok [I need the 4/5 patches from the last two days to push the vector
privatization and I need to find the 10 bugs I have in lnt (4 compile
time, 6 run time)]

> >================
> >Comment at: lib/Analysis/ScopInfo.cpp:633
> >@@ -620,5 +632,3 @@
> >    isl_map *S = const_cast<isl_map *>(Schedule);
> >-  isl_map *AccessRelation = getNewAccessRelation();
> >-  if (!AccessRelation)
> >-    AccessRelation = getAccessRelation();
> >+  isl_map *AccessRelation = getNewestAccessRelation();
> >    isl_space *Space = isl_space_range(isl_map_get_space(S));
> >----------------
> >grosser wrote:
> >>Having here getAccessRelation() (with the same semantics as now getNewestAccessRelation) seems more understandable. Updates to the access relation should not really affect the code all over polly.
> >Do you want getAccessRelation or getOriginalAccessRelation (wrt. your comment above)
> 
> If I am not mistaken this would become getAccessRelation() with the
> semantics of the current getNewestAccessRelation(). Or am I confused?
Shouldn't matter that's why I ask. But the way you describe it makes
sense.

-- 

Johannes Doerfert
Researcher / PhD Student

Compiler Design Lab (Prof. Hack)
Saarland University, Computer Science
Building E1.3, Room 4.26

Tel. +49 (0)681 302-57521 : doerfert at cs.uni-saarland.de
Fax. +49 (0)681 302-3065  : http://www.cdl.uni-saarland.de/people/doerfert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141010/935b7ad0/attachment.sig>