[PATCH] D109958: [LoopFlatten] Enable it by default

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/opt-pipeline.ll:164
+; GCN-O1-NEXT:       Flattens loops
+; GCN-O1-NEXT:       Memory SSA
 ; GCN-O1-NEXT:       Loop Pass Manager
----------------
aeubanks wrote:
> SjoerdMeijer wrote:
> > SjoerdMeijer wrote:
> > > nikic wrote:
> > > > I'm somewhat confused by what is going on here. Why do we now calculate MemorySSA and why does LoopUnroll get split into a separate LPM?
> > > I have no idea and accepted this as something the pass manager decided to do.... I am also confused about both things: I have no idea why we need to rerun Memory SSA, and don't see why LoopUnroll is now run separately.  I will look into this, see if I can get any wiser here....
> > In `lib/Passes/PassBuilderPipelines.cpp` we have this:
> > 
> >   if (EnableLoopFlatten)
> >     FPM.addPass(createFunctionToLoopPassAdaptor(LoopFlattenPass()));
> >   // The loop passes in LPM2 (LoopFullUnrollPass) do not preserve MemorySSA.
> >   // *All* loop passes must preserve it, in order to be able to use it.
> >   FPM.addPass(createFunctionToLoopPassAdaptor(std::move(LPM2),
> >                                               /*UseMemorySSA=*/false,
> >                                               /*UseBlockFrequencyInfo=*/false));
> > 
> > Since this is talking about MemorySSA this might be related, but there are so many things going on here and I am still looking, so don't know for certain. If e.g. @aeubanks has some tips or suggestions here, I would be happy to receive them. :)
> This is a legacy PM test.
> I'm not super familiar with the legacy PM, but it's probably something to do with the fact that the legacy loop flatten pass is a function pass and perhaps doesn't preserve some analyses?
> Could we perhaps just make this change for the new PM?
> I'm not super familiar with the legacy PM, but it's probably something to do with the fact that the legacy loop flatten pass is a function pass and perhaps doesn't preserve some analyses?

Yep, that's what I thought too, but didn't get to the bottom of it and can't "prove" it.

> Could we perhaps just make this change for the new PM?

Thanks for the suggestion, I am happy to enable LoopFlatten by default only for the new PM, so will look into that. Diverging is probably not ideal, but since the legacy PM is deprecated I am happy to do that. The alternative is to "accept" that this is what the legacy PM does, but looks like we are not happy with that, so again, will do this only for the new PM. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109958/new/

https://reviews.llvm.org/D109958