[PATCH] D22405: [PM] Add a generic 'repeat N times' pass wrapper to the new pass manager.

Sean Silva via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 3 00:12:44 PDT 2016


silvas added inline comments.

================
Comment at: test/Other/new-pass-manager.ll:432
@@ +431,3 @@
+; RUN:     | FileCheck %s --check-prefix=CHECK-REPEAT-LOOP-PASS
+; CHECK-REPEAT-LOOP-PASS: Starting llvm::Module pass manager run
+; CHECK-REPEAT-LOOP-PASS-NEXT: Running pass: ModuleToFunctionPassAdaptor
----------------
chandlerc wrote:
> silvas wrote:
> > Please don't just blindly check the exact output. Check for what you are looking for (e.g. a particular pass being run a particular number of times).
> > Same for the other tests.
> I'm not just blindly checking the full output.
> 
> At several points, I had bugs with too many intermediate pass managers. Having the precise output checked made it really obvious.
> 
> While I don't think we should have very many places that check the *exact* pipeline construction details in this way, I think it is reasonable to have roughly one place that surfaces any change in the pipeline structure so that the structure doesn't change in unintended ways. I could reduce the repeatition count to 2 to make this a bit less chatty if that would help, but not sure that it would.
> 
> (It is a bit unfortunate that I have to check the *analysis* runs here, and I actually thought about how to avoid that and didn't come up with anything better. It would end up being giant hunks of -NOT checks to blacklist everything that shouldn't go in the space the analysis runs did, and would still end up hard coding the analysis runs go in that space. This is just a fundamental problem with FileCheck based tests -- when you want the test to fail if *new* things show up, you end up hard coding more than you wanted.)
I disagree, but only mildly. If you feel strongly about keeping this feel free to commit as-is.


https://reviews.llvm.org/D22405





More information about the llvm-commits mailing list