This is an archive of the discontinued LLVM Phabricator instance.

[Polly][ScheduleOptimizer] Allow tiling after fusion
ClosedPublic

Authored by ttheodor on Mar 10 2017, 2:09 AM.

Details

Summary

In ScheduleOptimizer::isTileableBand(), allow the case in which
the band node's child is an isl_schedule_sequence_node and its
grandchildren isl_schedule_leaf_nodes. This case can arise when
two or more statements are fused by the isl scheduler.

The tile_after_fusion.ll test has two statements in separate
loop nests and checks whether they are tiled after being fused
when polly-opt-fusion equals "max".

Diff Detail

Event Timeline

ttheodor created this revision.Mar 10 2017, 2:09 AM
grosser edited edge metadata.Mar 12 2017, 12:58 AM

Very nice. I added a larger set of comments, but most are really minor. Can you update the patch accordingly, such that we can commit it?

lib/Transform/ScheduleOptimizer.cpp
444

Why do you start a new namespace here?

Also, please do not try to imitate the isl lowercase function names. LLVM uses CamelCase.

Also, this function only makes sense to be called on a node with a single child. Maybe we should assert this?

485

Please use CamelCase for variable names.The only lower_case we see here is due to isl function names. All the code we use should be CamelCase.

486

We only check the type of the child of the child, but never the child itself. Maybe it makes sense to check that the child type is a filter node as well?

494

It probably makes sense to move all this functionality in a function "bool isSimpleInnerMostBand()" which can also be documented.

Thanks for the patch!

Some minor comments from me:

  • Could you prefix the title with "[Polly]"?
  • Could you add a description what the test was written for?
ttheodor retitled this revision from [ScheduleOptimizer] Allow tiling after fusion to [Polly][ScheduleOptimizer] Allow tiling after fusion.Mar 12 2017, 11:07 AM
ttheodor updated this revision to Diff 91505.Mar 12 2017, 12:06 PM
  • Switch to CamelCase.
  • Move functionality to a new function called "isSimpleInnermostBand".
ttheodor edited the summary of this revision. (Show Details)Mar 12 2017, 12:08 PM
ttheodor edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.