[NewPM][LoopFullUnroll] Make LoopFullUnrollPass required

[aeubanks]

Summary

This is so loops should be fully unrolled are actually unrolled even when the function is optnone.

[asbirlea]

I'm assuming this is the desired behavior (pragma overwriting optnone).

[aeubanks]

Make LCSSA/LoopSimplify required, as previously discussed in https://reviews.llvm.org/D84977
optnone-opt.ll was broken without this due to an assert

[ychen]

This is to say optnone does not work for these 3 passes all the time. Could we do this in the optnone callback by checking the metadata of the branch inst in latch block? That is precisely what we meant here.

[aeubanks]

In D86005#2221998, @ychen wrote:

This is to say optnone does not work for these 3 passes all the time. Could we do this in the optnone callback by checking the metadata of the branch inst in latch block? That is precisely what we meant here.

Do you mean don't have the optnone pass instrumentation say that the pass should be skipped if it sees the proper metadata? I think the metadata is specific just to this one pass and checking it doesn't belong in the optnone pass instrumentation.
There may be some semantic meaning to fully unrolling a loop, and that should happen regardless of optnone/opt-bisect, whatever else.

[ychen]

In D86005#2222884, @aeubanks wrote:

In D86005#2221998, @ychen wrote:

This is to say optnone does not work for these 3 passes all the time. Could we do this in the optnone callback by checking the metadata of the branch inst in latch block? That is precisely what we meant here.

Do you mean don't have the optnone pass instrumentation say that the pass should be skipped if it sees the proper metadata?

Yes, that's what I meant because we're overriding optnone's decision based on "llvm.loop.unroll.full" here. The patch in its current shape means the three passes are immune to the optnone which seems stronger than necessary.

I think the metadata is specific just to this one pass and checking it doesn't belong in the optnone pass instrumentation.

Yeah, it is a little bit out of place. I'm running out of idea here. However, putting the logic there is very explicit about the intention.

There may be some semantic meaning to fully unrolling a loop, and that should happen regardless of optnone/opt-bisect, whatever else.

Fully unrolling loops is just a regular transformation pass. It happens to have a user knob that should be respected (pragma). I could not think of other scenarios that it is special but I'm not sure. @asbirlea thoughts?

[ebrevnov]

I think making 3 transformation passes immune to optnone ruins the whole idea. You really should have mechanism to set very specific and well documented overrides to the default.

[aeubanks]

Assuming that LoopFullUnrollPass is always required to run on anything with llvm.loop.unroll.full, it doesn't make sense to make this part of the optnone pass instrumentation since we'd have to add the check to all the pass instrumentations, not just optnone. e.g. opt-bisect should never skip the pass either. Instead ideally this would be part of isRequired(Pass&), and the pass would be able to look at the IR to determine if the pass is required. Then all 3 passes could check for the metadata.
We could try going down that route. I'm not sure that the extra code is worth the complexity just to handle LoopFullUnrollPass. The only other thing I can think of like this is alwaysinline, but the inliners don't have dependent passes so those can just be marked required.
Or we could just go with the current approach. LoopSimplify/LCSSA don't know if the pipeline has a required pass like LoopFullUnrollPass, so if we go with this approach I think marking them as required is necessary for this approach.

[ychen]

In D86005#2231214, @aeubanks wrote:

Assuming that LoopFullUnrollPass is always required to run on anything with llvm.loop.unroll.full, it doesn't make sense to make this part of the optnone pass instrumentation since we'd have to add the check to all the pass instrumentations, not just optnone. e.g. opt-bisect should never skip the pass either.

If llvm.loop.unroll.full is strong enough that it should override all existing and future pass skipping callbacks' decision, we could either just add it to all the relevant callbacks (probably adding some utility functions) or we just put it in PassInstrumentation::runBeforePass. I think it is more important to adhere to the separation of concerns and let pass do transformation while let PassInstrumentation handles all pass skipping. Another reason (not that it matters here) for PassInstrumentation to handle pass skipping is that it could maintain pipeline execution state while pass itself could not. i.e. you could say the third run of foo pass should be skipped.

We could try going down that route. I'm not sure that the extra code is worth the complexity just to handle LoopFullUnrollPass. The only other thing I can think of like this is alwaysinline, but the inliners don't have dependent passes so those can just be marked required.

Maybe I miss some details. But I would think it shouldn't be more complex than the current approach.

[aeubanks]

In D86005#2231360, @ychen wrote:

In D86005#2231214, @aeubanks wrote:

Assuming that LoopFullUnrollPass is always required to run on anything with llvm.loop.unroll.full, it doesn't make sense to make this part of the optnone pass instrumentation since we'd have to add the check to all the pass instrumentations, not just optnone. e.g. opt-bisect should never skip the pass either.

If llvm.loop.unroll.full is strong enough that it should override all existing and future pass skipping callbacks' decision, we could either just add it to all the relevant callbacks (probably adding some utility functions) or we just put it in PassInstrumentation::runBeforePass. I think it is more important to adhere to the separation of concerns and let pass do transformation while let PassInstrumentation handles all pass skipping. Another reason (not that it matters here) for PassInstrumentation to handle pass skipping is that it could maintain pipeline execution state while pass itself could not. i.e. you could say the third run of foo pass should be skipped.

We could try going down that route. I'm not sure that the extra code is worth the complexity just to handle LoopFullUnrollPass. The only other thing I can think of like this is alwaysinline, but the inliners don't have dependent passes so those can just be marked required.

Maybe I miss some details. But I would think it shouldn't be more complex than the current approach.

Looking back at the legacy PM passes for LCSSA and LoopSimplify, they ignore optnone/opt-bisect. Interestingly though, there doesn't seem to be a legacy PM version of LoopFullUnrollPass. There's only a LoopUnroll pass which does respect optnone/opt-bisect. So actually llvm.loop.unroll.full doesn't work with optnone in the legacy PM. And to be more general, I think none of the loop unroll pragmas [1] work under -O0. So this change shouldn't be necessary, sorry for the back and forth.
Maybe the optnone in FullUnroll.ll is not the true intention of the test added in https://reviews.llvm.org/D71687? I'll send out another patch and abandon this one.

[1]: https://clang.llvm.org/docs/AttributeReference.html#pragma-unroll-pragma-nounroll

llvm/include/llvm/Transforms/Scalar/LoopUnrollPass.h

 public:
   explicit LoopFullUnrollPass(int OptLevel = 2, bool OnlyWhenForced = false,
                               bool ForgetSCEV = false)
       : OptLevel(OptLevel), OnlyWhenForced(OnlyWhenForced),
         ForgetSCEV(ForgetSCEV) {}
 
   PreservedAnalyses run(Loop &L, LoopAnalysisManager &AM,
                         LoopStandardAnalysisResults &AR, LPMUpdater &U);
+  static bool isRequired() { return true; }
 };
 
 /// A set of parameters used to control various transforms performed by the
 /// LoopUnroll pass. Each of the boolean parameters can be set to:
 ///      true - enabling the transformation.
 ///      false - disabling the transformation.
 ///      None - relying on a global default.
 ///

llvm/include/llvm/Transforms/Utils/LCSSA.h

 #include "llvm/IR/PassManager.h"
 
 namespace llvm {
 
 /// Converts loops into loop-closed SSA form.
 class LCSSAPass : public PassInfoMixin<LCSSAPass> {
 public:
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
+  static bool isRequired() { return true; }
 };
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_UTILS_LCSSA_H

llvm/include/llvm/Transforms/Utils/LoopSimplify.h

 class LoopInfo;
 class MemorySSAUpdater;
 class ScalarEvolution;
 
 /// This pass is responsible for loop canonicalization.
 class LoopSimplifyPass : public PassInfoMixin<LoopSimplifyPass> {
 public:
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
+  static bool isRequired() { return true; }
 };
 
 /// Simplify each loop in a loop nest recursively.
 ///
 /// This takes a potentially un-simplified loop L (and its children) and turns
 /// it into a simplified loop nest with preheaders and single backedges. It will
 /// update \c DominatorTree, \c LoopInfo, \c ScalarEvolution and \c MemorySSA
 /// analyses if they're non-null, and LCSSA if \c PreserveLCSSA is true.
 bool simplifyLoop(Loop *L, DominatorTree *DT, LoopInfo *LI, ScalarEvolution *SE,
                   AssumptionCache *AC, MemorySSAUpdater *MSSAU,
                   bool PreserveLCSSA);
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_UTILS_LOOPSIMPLIFY_H

llvm/test/Feature/optnone-opt.ll

 ; LOOP-DAG: Skipping pass 'Recognize loop idioms'
 ; LOOP-DAG: Skipping pass 'Reroll loops'
 ; LOOP-DAG: Skipping pass 'Rotate Loops'
 ; LOOP-DAG: Skipping pass 'Simplify instructions in loops'
 ; LOOP-DAG: Skipping pass 'Unroll loops'
 ; LOOP-DAG: Skipping pass 'Unswitch loops'
 ; LoopPassManager should not be skipped over an optnone function
 ; NPM-LOOP-NOT: Skipping pass: PassManager
-; NPM-LOOP-DAG: Skipping pass: LoopSimplifyPass on {{.*}}foo
-; NPM-LOOP-DAG: Skipping pass: LCSSAPass
 ; NPM-LOOP-DAG: Skipping pass: IndVarSimplifyPass
 ; NPM-LOOP-DAG: Skipping pass: SimpleLoopUnswitchPass
-; NPM-LOOP-DAG: Skipping pass: LoopUnrollPass
+; NPM-LOOP-DAG: Skipping pass: LoopUnrollPass on {{.*}}foo
 ; NPM-LOOP-DAG: Skipping pass: LoopStrengthReducePass
 ; NPM-LOOP-DAG: Skipping pass: LoopDeletionPass
 ; NPM-LOOP-DAG: Skipping pass: LICMPass
 ; NPM-LOOP-DAG: Skipping pass: LoopIdiomRecognizePass
 ; NPM-LOOP-DAG: Skipping pass: LoopInstSimplifyPass

llvm/test/Transforms/LoopUnroll/FullUnroll.ll

 ; RUN: opt -passes='loop-unroll-full' -disable-verify --mtriple x86_64-pc-linux-gnu -S -o - %s | FileCheck %s
+; RUN: opt -enable-npm-optnone -passes='loop-unroll-full' -disable-verify --mtriple x86_64-pc-linux-gnu -S -o - %s | FileCheck %s
 
 ; This checks that the loop full unroller will fire in the new pass manager
 ; when forced via #pragma in the source (or annotation in the code).
 
 ; Completely unroll the inner loop
 ; CHECK-LABEL: @foo
 ; CHECK: br i1
 ; CHECK-NOT: br i1