[PATCH] D37010: [Polly][PM] Properly require and preservation of OptimizationRemarkEmitter.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/CodeGen/CodeGeneration.cpp:315
     // FIXME: We do not yet add regions for the newly generated code to the
     //        region tree.
   }
----------------
bollu wrote:
> Meinersbur wrote:
> > annanay25 wrote:
> > > Since RegionInfo is no longer preserved, do we need this?
> > 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.
> 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?
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.


https://reviews.llvm.org/D37010