[LoopFlatten] Mark loop analyses as perserved

[nikic]

Summary

LoopFlatten does preserve loop analyses (DT, LI and SCEV), but currently doesn't mark them as preserved in the NewPM (the are marked as preserved in the LegacyPM). I think this doesn't really have an effect in the end because the loop pass adaptor will just assume they're preserved anyway, but let's be explicit about this for the sake of clarity.

[aeubanks]

llvm/lib/Transforms/Scalar/LoopFlatten.cpp

665

separately, this looks like it's not telling LPMUpdater that a loop has been deleted which is an issue

[nikic]

llvm/lib/Transforms/Scalar/LoopFlatten.cpp

665

I think this is actually fine, because this is a LoopNest pass, in which case LPMUpdater only has to be informed about the removal of top-level loops.

[aeubanks]

llvm/lib/Transforms/Scalar/LoopFlatten.cpp

665

If we delete a Loop, then allocate another Loop at the same address, we may fetch an invalid cached analysis later on. LPMUpdater::markLoopAsDeleted() clears cached analyses for that Loop.

[nikic]

llvm/lib/Transforms/Scalar/LoopFlatten.cpp

665

After looking a bit more closely, here's the problem:

• For LoopNest passes, the loop pass adaptor will magically go into LoopNestMode if the LPM only contains loop nest passes.
• In LoopNestMode, only the top level loops are visited and trying to mark a non-top-level loop as deleted will assert (https://github.com/llvm/llvm-project/blob/bd5befb55087199ee1c0c3e344847cd06a2ca839/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h#L284).
• In non-LoopNestMode you do need to mark non-top-level loops for the reason you mentioned.

However, as the pass doesn't control whether it gets used in LoopNestMode or not, this means that either you are forbidden from invalidating other loops (only LoopNest passes scheduled) or you are required to invalidate other loops (non-LoopNest passes scheduled as well). Something seems broken here.

Here's my test patch: https://gist.github.com/nikic/1a65b8cb384bb7e00084279255b93e65 It will currently assert for standalone uses of -loop-flatten because those use LoopNestMode.

[nikic]

llvm/lib/Transforms/Scalar/LoopFlatten.cpp

665

I've put up D111350 to add the markLoopAsDeleted() call while relaxing the assertion in LPMUpdater.

llvm/lib/Transforms/Scalar/LoopFlatten.cpp

LLVM_DEBUG(dbgs() << "Replacing: "; V->dump();
                dbgs() << "with:      "; OuterValue->dump());
     V->replaceAllUsesWith(OuterValue);
   }
 
   // Tell LoopInfo, SCEV and the pass manager that the inner loop has been
   // deleted, and any information that have about the outer loop invalidated.
   SE->forgetLoop(FI.OuterLoop);
   SE->forgetLoop(FI.InnerLoop);
   LI->erase(FI.InnerLoop);
 
   // Increment statistic value.
   NumFlattened++;
 
   return true;
 }
 
 static bool CanWidenIV(FlattenInfo &FI, DominatorTree *DT, LoopInfo *LI,
PreservedAnalyses LoopFlattenPass::run(LoopNest &LN, LoopAnalysisManager &LAM,
   // in simplified form, and also needs LCSSA. Running
   // this pass will simplify all loops that contain inner loops,
   // regardless of whether anything ends up being flattened.
   Changed |= Flatten(LN, &AR.DT, &AR.LI, &AR.SE, &AR.AC, &AR.TTI);
 
   if (!Changed)
     return PreservedAnalyses::all();
 
-  return PreservedAnalyses::none();
+  return getLoopPassPreservedAnalyses();
 }
 
 namespace {
 class LoopFlattenLegacyPass : public FunctionPass {
 public:
   static char ID; // Pass ID, replacement for typeid
   LoopFlattenLegacyPass() : FunctionPass(ID) {
     initializeLoopFlattenLegacyPassPass(*PassRegistry::getPassRegistry());