Page MenuHomePhabricator

[Polly] Update the documentation on how the packing transformation is implemented
ClosedPublic

Authored by gareevroman on Dec 21 2016, 3:16 AM.

Diff Detail

Event Timeline

gareevroman retitled this revision from to [Polly] Update the documentation on how the packing transformation is implemented.
gareevroman updated this object.
gareevroman added a subscriber: pollydev.

Why is the _access_ relation mapping to a 9 dimensional space. You talk about a _schedule_ here, right?

Right.

The access relation seems to implement a modulo constraint. It probably is easier to read if we write the modulo explicitly. I suggest to just submit a patch with this comment, then Michael and me can read over it and see if we still have some comments.

I've updated it.

Thanks for the comments!

Meinersbur added inline comments.Jan 11 2017, 8:02 AM
lib/Transform/ScheduleOptimizer.cpp
894–899

This is a mouthful to read and I cannot take any additional information from it. If show this here to illustrate the space of these isl_maps, I'd suggest to shorten it to something like { Domain[] -> Element[] } and { Domain[] -> Scatter[] } like I often do for DeLICM when the spaces are not obvious.

If it is for showing a numerical example, this is way to complicated. It would be better if you would construct an illustrative example by hand that only contains the relevant parts instead of dumping something from a test case.

904–906

How did we get to this?

{ Stmt_for_body6[i0, i1, i2] -> [o0, o1, o2] ... } is a scatter function as taken by setNewAccessRelation(), but the phrase uses the term "access location" which I would be something like { Stmt_for_body6[] -> MemRef_CopyA[] }.

907

setNewAccessRelation is only the setter method updating the data, about as interesting as MA->NewAccessRelation = NewAccessRelation. It would be more interesting who calls it.

grosser edited edge metadata.Jan 26 2017, 1:55 AM

Hi Roman, it might make sense to update this according to what we now have in our paper.

lib/Transform/ScheduleOptimizer.cpp
891

access relation

894–899

Hi Roman,

AFAIR the access functions in this comment can indeed be further simplified, no?

Meinersbur added inline comments.Jan 27 2017, 7:53 AM
lib/Transform/ScheduleOptimizer.cpp
904–906

I mixed up something here. It has the space of a scatter function, but probably only the target array (eg. Packed_A) is missing here (and setNewAccessRelation() does not accept scatter functions)

Thanks for the comments! I'll try to address them soon.

Update according to the comments.

This is a mouthful to read and I cannot take any additional information from it. If show this here to illustrate the space of these isl_maps, I'd suggest to shorten it to something like { Domain[] -> Element[] } and { Domain[] -> Scatter[] } like I often do for DeLICM when the spaces are not obvious.

I've tried to simplify the scheduling function and access relations. What do you think about it?

grosser accepted this revision.Jan 29 2017, 1:43 AM

Perfect!

This revision is now accepted and ready to land.Jan 29 2017, 1:43 AM
This revision was automatically updated to reflect the committed changes.