Page MenuHomePhabricator

[Polly][PM] Properly require and preservation of OptimizationRemarkEmitter.

Authored by Meinersbur on Aug 22 2017, 6:06 AM.



Properly require and preserve the OptimizationRemarkEmitter for use in ScopPass. Previously one had to get the ORE from ScopDetection because CodeGeneration did not mark it as preserved. It would need to be recomputed which results in the legacy PM to throw away all previous SCoP analysis.

This also changes the implementation of ScopPass::getAnalysisUsage to not unconditionally preserve all passes, but only those needed to be preserved by any SCoP pass (at least when using the legacy PM). This allows invalidating DependenceInfo (and IslAstInfo) in case the pass would cause them to change (e.g. JSONImporter, OpTree, DeLICM, MaximalArrayExpansion, IslScheduleOptimizer)

Diff Detail


Event Timeline

Meinersbur created this revision.Aug 22 2017, 6:06 AM
annanay25 added inline comments.
315 ↗(On Diff #112160)

Since RegionInfo is no longer preserved, do we need this?

3532 ↗(On Diff #112160)

Same as above.

niosega edited edge metadata.Aug 22 2017, 8:21 AM

This works well with MSE. Thanks a lot for this patch.

Meinersbur added inline comments.Aug 22 2017, 8:57 AM
315 ↗(On Diff #112160)

This is still an issue.

We mark RegionInfo and LoopInfo as preserved, but only such that their verifications do not fail. The code generated by Polly will have now regions and loops in it, but we do not update Loop/RegionInfo to include these new loops/regions. That is, the newly generated code looks 'flat' without structure. Its good enough for Polly because we do not need the information for the other SCoPs in the function and do not want to re-run Polly on SCoPs in the generated code.

There is a barrier pass after Polly that forces the LoopInfo/RegionInfo to be released. Any pass after that will get fresh new RegionInfo/LoopInfo redetection. At least in the legacy pass manager. Yes, it's a bad hack.

Meinersbur added inline comments.Aug 22 2017, 8:59 AM
55 ↗(On Diff #112160)

Btw, RegionInfo is still preserved here.

bollu added inline comments.Aug 22 2017, 9:36 AM
315 ↗(On Diff #112160)

Since we edit the AST in controlled ways, can we not fix this issue? I'm asking if there is something inherent that stops from keeping Region and Loop info up-to-date?

If not, we can incrementally fix this, correct?

Meinersbur added inline comments.Aug 23 2017, 7:18 AM
315 ↗(On Diff #112160)

LoopInfo is relatively easy. We know exactly all the loops in isl's ast, therefore we could just update the LoopInfo using the blocks we generated the loop for.

RegionInfo is harder. ScopDetection actually modifies the regions. I also see no obvious way to find all regions in the generated code as a fresh run of RegionInfo would detect. That is, a pass using RegionInfo might do different things if run after Polly or after a fresh run of RegionInfo. Stuff like that is hard to debug. Since there is no consumer of RegionInfo after Polly anyway, I'd say it's not worth it.

Sure it can be fixed, it just wasn't a priority yet. The issues with DependenceInfo seem to be more severe to me.

simbuerg accepted this revision.Aug 28 2017, 2:39 AM


This revision is now accepted and ready to land.Aug 28 2017, 2:39 AM
This revision was automatically updated to reflect the committed changes.