Page MenuHomePhabricator

[NewPM][LoopFullUnroll] Make LoopFullUnrollPass required
AbandonedPublic

Authored by aeubanks on Aug 14 2020, 4:16 PM.

Details

Summary

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

Diff Detail

Event Timeline

aeubanks created this revision.Aug 14 2020, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2020, 4:16 PM
aeubanks requested review of this revision.Aug 14 2020, 4:16 PM
asbirlea accepted this revision.Aug 15 2020, 9:41 AM

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

This revision is now accepted and ready to land.Aug 15 2020, 9:41 AM
aeubanks updated this revision to Diff 286073.Aug 17 2020, 10:39 AM

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

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.

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 added a comment.Aug 17 2020, 5:48 PM

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?

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.

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 added a comment.Aug 21 2020, 3:07 PM

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.

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

aeubanks abandoned this revision.Aug 28 2020, 11:29 AM