This is an archive of the discontinued LLVM Phabricator instance.

[Polly][ScheduleOptimizer] Allow tiling after fusion

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



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?

444 ↗(On Diff #91299)

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?

472 ↗(On Diff #91299)

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.

473 ↗(On Diff #91299)

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?

481 ↗(On Diff #91299)

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.