Page MenuHomePhabricator

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

Authored by jdoerfert on Oct 9 2014, 11:07 AM.

Diff Detail


Event Timeline

jdoerfert updated this revision to Diff 14663.Oct 9 2014, 11:07 AM
jdoerfert retitled this revision from to [Polly][Refactor][NFC] Simplify and clean the handling of (new) access relations.
jdoerfert added subscribers: Restricted Project, Unknown Object (MLST).
dpeixott accepted this revision.Oct 9 2014, 12:10 PM
dpeixott edited edge metadata.
dpeixott added inline comments.
245 ↗(On Diff #14663)

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.

This revision is now accepted and ready to land.Oct 9 2014, 12:10 PM
jdoerfert added inline comments.Oct 9 2014, 1:19 PM
245 ↗(On Diff #14663)

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.


grosser added inline comments.Oct 10 2014, 7:11 AM
245 ↗(On Diff #14663)

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.

410 ↗(On Diff #14663)

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.

633 ↗(On Diff #14663)

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.

192 ↗(On Diff #14663)


jdoerfert added inline comments.Oct 10 2014, 7:24 AM
245 ↗(On Diff #14663)

I will do that.

410 ↗(On Diff #14663)

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

633 ↗(On Diff #14663)

Do you want getAccessRelation or getOriginalAccessRelation (wrt. your comment above)

jdoerfert closed this revision.Oct 13 2014, 6:08 AM
jdoerfert updated this revision to Diff 14798.

Closed by commit rL219612 (authored by @jdoerfert).