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

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: lib/Passes/PassBuilder.cpp:278
@@ +277,3 @@
+static Optional<int> parseRepeatPassName(StringRef Name) {
+  if (!Name.consume_front("repeat<") || !Name.consume_back(">"))
+    return None;
----------------
silvas wrote:
> It worries me that we're going down the path of "no proper lexer". D22724 improved the state of things somewhat.
> 
> I just have scars. Every time I've decided to not put a proper lexer or tokenization step in front of any kind of "parsing" I have always regretted it down the road.
> 
> It would be great if you could improve this in future patches (this would also probably make source location reporting much easier too since the token can store a source location).
> 
> Just an FYI. I think this is fine for now.
Yea, I don't completely disagree and expect at some point this will become more formalized. I'm mostly trying to follow a lazy design principle so that we don't over-engineer this early on because I'm not yet sure what some of the constraints will be. As things settle in terms of functionality, it'll probably become more worthwhile to then harden this properly.

================
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
----------------
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.)


https://reviews.llvm.org/D22405