This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Allow passes to never be skipped
ClosedPublic

Authored by ychen on Jun 22 2020, 8:11 PM.

Details

Summary

A pass declares itself unskippable by defining a method static bool isRequired().

Also, this patch makes pass managers and adaptor passes required (unskippable).

PassInstrumentation before-pass-callbacks could be used to skip passes by returning false.
However, some passes should not be skipped at all. Especially so for special-purpose passes such as pass managers and adaptor passes since if they are skipped for any reason, the passes contained by them would also be skipped ignoring contained passes's return value of isRequired().

Diff Detail

Event Timeline

ychen created this revision.Jun 22 2020, 8:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2020, 8:11 PM
ychen retitled this revision from [NewPM] Make adaptor passes unskippable from PassInstrumentation callbacks to [NewPM] Make adaptor passes unskippable.Jun 22 2020, 8:16 PM
ychen edited the summary of this revision. (Show Details)
ychen retitled this revision from [NewPM] Make adaptor passes unskippable to [NewPM] Make adaptor passes for pass managers unskippable.Jun 22 2020, 8:22 PM
ychen edited the summary of this revision. (Show Details)
ychen updated this revision to Diff 272625.Jun 22 2020, 11:27 PM
  • Update
ychen planned changes to this revision.Jun 23 2020, 8:20 AM
ychen marked an inline comment as done.
ychen added inline comments.
llvm/include/llvm/IR/PassManagerInternal.h
106

Using ifdef may not work, I will fix this.

ychen updated this revision to Diff 272878.Jun 23 2020, 5:41 PM
  • Simplify by letting pass optionally implement static bool isSkippable(). This also make the skipping logic internal to PassInstrumentation, no change is needed in PassManger itself for the sake of performing skipping.
  • All the implementation are through the type-erased API. The type traits in PassInstrumentation is for handling cases where adaptor passes instantiated for regular passes.
ychen retitled this revision from [NewPM] Make adaptor passes for pass managers unskippable to [NewPM] Make PMs and adaptor passes for PMs unskippable.Jun 23 2020, 5:42 PM
ychen edited the summary of this revision. (Show Details)
ychen edited the summary of this revision. (Show Details)Jun 24 2020, 9:27 AM
ychen added a comment.Jun 24 2020, 3:19 PM

By Eric's suggestion, I'll add a test case to make the intent more clear.

ychen updated this revision to Diff 273875.Jun 26 2020, 6:17 PM
  • Add test. The test covers all changes except when a user pass is wrapped

in a adaptor pass directly(without nested pass manager). So part of the type
traits in PassInstrumentation.h is not tested. The reason is PassInstrumentation
unit tests are relying on parsePassPipeline which could not parse adaptor-wrapped user passes.
See D82698.

ychen updated this revision to Diff 274231.Jun 29 2020, 2:05 PM
  • Seems this could clear the TODO in PassInstrumentation.cpp (provided this implementation is in favor.)

I think I prefer https://reviews.llvm.org/D83575 over this, this uses too much template metaprogramming for my liking. WDYT?

llvm/include/llvm/Analysis/CGSCCPassManager.h
359

This is saying that only a ModuleToPostOrderCGSCCPassAdaptor around a CGSCCPassManager isn't skippable, all other ModuleToPostOrderCGSCCPassAdaptor are? What about a wrapper around a normal CGSCC pass that isn't skippable?

ychen marked an inline comment as done.Jul 10 2020, 1:00 PM

I think I prefer https://reviews.llvm.org/D83575 over this, this uses too much template metaprogramming for my liking. WDYT?

I'm happy if either this or D83575 are landed and slightly in favor of this. The difference between this and D83575 is that if we want the pass to implement an extra require method all the time. I choose not too because I think it is less pervasive and I imagine the use cases for require are limited. I'm kind of want to hide this detail from most pass developers so they don't need to worry about this in most cases. The template stuff is used so we don't mandate a require method all the time and the templates could be simplified using is_detected.

Other than the points I made I think it is perfectly reasonable to pursue D83575. For one thing, I'd prefer require over skippable. And it requires less code diff FWIW.

Anyway since we're authors of the alternatives and I'm probably biased. It would be a good thing to solicit other opinions. Thoughts?

llvm/include/llvm/Analysis/CGSCCPassManager.h
359

What about a wrapper around a normal CGSCC pass that isn't skippable?

Good point! I was not considering the case of normal passes declaring require. I think it should return false unconditionally here to mean the adaptor itself is always required.

I think I prefer https://reviews.llvm.org/D83575 over this, this uses too much template metaprogramming for my liking. WDYT?

I'm happy if either this or D83575 are landed and slightly in favor of this. The difference between this and D83575 is that if we want the pass to implement an extra require method all the time. I choose not too because I think it is less pervasive and I imagine the use cases for require are limited. I'm kind of want to hide this detail from most pass developers so they don't need to worry about this in most cases. The template stuff is used so we don't mandate a require method all the time and the templates could be simplified using is_detected.

Other than the points I made I think it is perfectly reasonable to pursue D83575. For one thing, I'd prefer require over skippable. And it requires less code diff FWIW.

Anyway since we're authors of the alternatives and I'm probably biased. It would be a good thing to solicit other opinions. Thoughts?

Alina, Hans, any preference between these two?

I'm inclined to favor having the isRequired() method optional with the SFINAE in this patch.
It's up to you two which patch to actually keep, but I'd say update this patch, with renaming to isRequired() (and flipping the bool value), adding the default to true in isRequired() for all adaptors and if, as you mentioned the diff can be simplified, great, but if not, this one makes sense.

Alright let's go with this one then, with skippable -> required.

llvm/include/llvm/Analysis/CGSCCPassManager.h
359

Maybe we can just return whether or not the CGSSC pass is skippable? If that's doable. That's a small optimization.

ychen updated this revision to Diff 278659.Jul 16 2020, 9:16 PM
  • skippable -> isRequired
  • simplify SFINAE with is_detected
ychen updated this revision to Diff 278661.EditedJul 16 2020, 10:10 PM
  • 'required' to 'isRequired' in PassInstrumentation.h
aeubanks accepted this revision.Jul 17 2020, 9:43 AM

lgtm, thanks!

This revision is now accepted and ready to land.Jul 17 2020, 9:43 AM
ychen retitled this revision from [NewPM] Make PMs and adaptor passes for PMs unskippable to [NewPM] Allow passes to never be skipped.Jul 17 2020, 4:00 PM
ychen edited the summary of this revision. (Show Details)
ychen updated this revision to Diff 279056.Jul 18 2020, 10:29 PM
ychen edited the summary of this revision. (Show Details)
  • fix a typo in comment

This change causes problems to us. Even though "enable-npm-optnone" is false by default it affects us because we use custom pass instrumentation callback which causes passes to be skipped under some conditions.

Here is a short reproducer for the problem:

opt -enable-new-pm -enable-npm-optnone -passes=licm test.ll
cat test.ll

define i32 @test1(i32 %a) #0 {
entry:

br label %body

body:

%i.0 = phi i32 [ 0, %entry ], [ %inc, %body ]
%inc = add nsw i32 %i.0, 1
%cmp = icmp slt i32 %i.0, %a
br i1 %cmp, label %body, label %end

end:

ret i32 %inc

}

attributes #0 = { optnone noinline }

It fails with assert(L->isRecursivelyLCSSAForm(LAR.DT, LI) && "Loops must remain in LCSSA form!") at FunctionToLoopPassAdaptor::run:311. The problem here is that LoopSimplify and LCSAA passes of LoopCanonicalizationFPM was skipped and we hit the assert. Interestingly, even if I move "if (!PI.runBeforePass<Loop>(Pass, *L)) continue;" from line 318 to be above the assert we still hit this assert. That happens because loop pass is PassManager itself and is not skipped. One possible solution which comes to my mind would be to run LCSSA verification pass by means of pass manger so it would be skipped by the same mechanism.

This change causes problems to us. Even though "enable-npm-optnone" is false by default it affects us because we use custom pass instrumentation callback which causes passes to be skipped under some conditions.

Here is a short reproducer for the problem:

opt -enable-new-pm -enable-npm-optnone -passes=licm test.ll
cat test.ll

define i32 @test1(i32 %a) #0 {
entry:

br label %body

body:

%i.0 = phi i32 [ 0, %entry ], [ %inc, %body ]
%inc = add nsw i32 %i.0, 1
%cmp = icmp slt i32 %i.0, %a
br i1 %cmp, label %body, label %end

end:

ret i32 %inc

}

attributes #0 = { optnone noinline }

It fails with assert(L->isRecursivelyLCSSAForm(LAR.DT, LI) && "Loops must remain in LCSSA form!") at FunctionToLoopPassAdaptor::run:311. The problem here is that LoopSimplify and LCSAA passes of LoopCanonicalizationFPM was skipped and we hit the assert. Interestingly, even if I move "if (!PI.runBeforePass<Loop>(Pass, *L)) continue;" from line 318 to be above the assert we still hit this assert. That happens because loop pass is PassManager itself and is not skipped. One possible solution which comes to my mind would be to run LCSSA verification pass by means of pass manger so it would be skipped by the same mechanism.

Thanks. Confirmed. Considering it is a debugging feature like pass logging, I think I'm going to add another BeforeNonSkipped callback (D84774) to run this since it is probably useful to verify other IR unit before/after a real pass in the future.

This change causes problems to us. Even though "enable-npm-optnone" is false by default it affects us because we use custom pass instrumentation callback which causes passes to be skipped under some conditions.

Here is a short reproducer for the problem:

opt -enable-new-pm -enable-npm-optnone -passes=licm test.ll
cat test.ll

define i32 @test1(i32 %a) #0 {
entry:

br label %body

body:

%i.0 = phi i32 [ 0, %entry ], [ %inc, %body ]
%inc = add nsw i32 %i.0, 1
%cmp = icmp slt i32 %i.0, %a
br i1 %cmp, label %body, label %end

end:

ret i32 %inc

}

attributes #0 = { optnone noinline }

It fails with assert(L->isRecursivelyLCSSAForm(LAR.DT, LI) && "Loops must remain in LCSSA form!") at FunctionToLoopPassAdaptor::run:311. The problem here is that LoopSimplify and LCSAA passes of LoopCanonicalizationFPM was skipped and we hit the assert. Interestingly, even if I move "if (!PI.runBeforePass<Loop>(Pass, *L)) continue;" from line 318 to be above the assert we still hit this assert. That happens because loop pass is PassManager itself and is not skipped. One possible solution which comes to my mind would be to run LCSSA verification pass by means of pass manger so it would be skipped by the same mechanism.

Thanks. Confirmed. Considering it is a debugging feature like pass logging, I think I'm going to add another BeforeNonSkipped callback (D84774) to run this since it is probably useful to verify other IR unit before/after a real pass in the future.

+1, thanks!