[PATCH] D86005: [NewPM][LoopFullUnroll] Make LoopFullUnrollPass required

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In D86005#2231360 <https://reviews.llvm.org/D86005#2231360>, @ychen wrote:

> In D86005#2231214 <https://reviews.llvm.org/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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86005