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

Johannes Doerfert doerfert at cs.uni-saarland.de
Fri Oct 10 07:24:56 PDT 2014

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.

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].

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)


More information about the llvm-commits mailing list