Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[Refactor] Replace RegionPasses by FunctionPasses

Authored by jdoerfert on Nov 4 2015, 2:38 PM.


The main change is the switch to function instead of region passes.

However, due to the changed interface and the fact that analysis now
run on multiple SCoPs before their results are queried, other
adjustments were needed. These additional changes include:
  - Allow passes to handle multiple SCoPs at once, e.g., map the SCoPs
    to the information formerly stored as members in the pass.
  - Parametrize functions with the SCoP in question as there is not
    only one at a time anymore.
  - Update the tests not to look for the region pass manager output
  - Add a explicit ScopPass initialization function that is called
    once per function. This way analysis passes do not need to be
    queried for each SCoP.
  - Remap values between SCoPs in a function correctly.

There is a unit test failing because we split the region and that causes
dominance problems for other SCoPs. However, I'll wait till we fix the two
open dominance bugs (including the region splitting one) before I fix this.

Diff Detail

Event Timeline

jdoerfert updated this revision to Diff 39261.Nov 4 2015, 2:38 PM
jdoerfert retitled this revision from to [Refactor] Replace RegionPasses by FunctionPasses.
jdoerfert updated this object.
jdoerfert added reviewers: grosser, Meinersbur.
jdoerfert added a subscriber: Restricted Project.
grosser edited edge metadata.Jan 26 2016, 9:57 AM

As I already mentioned in discussions on the mailing list, I am concerned about moving to function passes, as building up all scops ahead of time then code generating the first may invalidate later scops. We had this problem maybe two years ago, which is why we moved away from this first-build-all-then-codegen-all model. I could see us using a different approach then what we do today, but I think there are various possible approaches that we should consider:

  1. Model All - Then build all, but leave pass structure as today (this patch)
  2. Have a single function pass that iteratively schedules scop detection / scop modeling / code generation just using library calls to implement this functionality
  3. Keep the pass structure, but use on-demand scop modeling and caching similar to how the new pass manager caches objects.

It is not yet clear to me which approach is the best and I assume that the new pass manger will affect the decisions we take. Hence I suggest two things:

  1. Most of the changes in ScopInfo go very much in a direction that will be useful for all of these approaches. What we want is to have a class that constructs a scop for a region and returns a scop object that is not connected with the ScopInfo pass. I suggest to introduce such a class to ensure that most of these changes do not bitrot and to reduce the diff needed to play around with these solutions.
  1. I suggest to look into the new pass manager infrastructure and use this as a testbed to experiment with these different solutions. This should give us both necessary knowledge about the new pass manager as well as confidence on the stability of the different approaches without breaking the current behavior.
Meinersbur edited edge metadata.Jan 27 2016, 7:45 AM

To add to the discussion: In a Polly phone call, Zino discussed that we might go directly to CallGraphSCCPass, as fair as I remember to do something with inlining.

This would also allow us to generate code into a newly created function s.t. we do not need to modify the original code before all transformations are done (also to get rid of the exit block PHI during code generation).

Johannes, what is the motivation to have this past?

Hi Johannes,

it seems first parts of this patch have been addressed with some of Utpal's recent commits and there is more work you and Utpal are doing that is going in the very same direction. Would it make sense to close this review and continue the discussion in the new reviews?

PS: Currently trying to clean up some of the review history

jdoerfert abandoned this revision.Jun 13 2016, 3:22 AM

Abandoned and redone in the GSoC.