Being lazy with printing the banner seems hard to reason with, we should print it unconditionally first (it could also lead to duplicate banners if we have multiple functions in -filter-print-funcs). The printIR() functions were doing too many things. I separated out the call from PrintPassInstrumentation since we were essentially doing two completely separate things in printIR() from different callers. There were multiple ways to generate the name of some IR. That's all been moved to getIRName(). The printing of the IR name was also inconsistent, now it's always "IR Dump on $foo" where "$foo" is the name. For a function, it's the function name. For a loop, it's what's printed by Loop::print(), which is more detailed. For an SCC, it's the list of functions in parentheses. For a module it's "[module]", to differentiate between a possible SCC with a function called "module". To preserve D74814, we have to check if we're going to print anything at all first. This is unfortunate, but I would consider this a special case that shouldn't be handled in the core logic.
Details
- Reviewers
jamieschmeiser - Commits
- rGc8f0a7c215ab: [NewPM] Cleanup IR printing instrumentation
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for doing this. There are a couple of comments/asserts that were missed...The only real thing is that the functionality of unwrapping Any is repeated many times... perhaps a function taking lambdas for what to do with each type of IR would be useful, probably in the header with Any? WDYT?
llvm/lib/Passes/StandardInstrumentations.cpp | ||
---|---|---|
195–196 | Remove second sentence of this comment. It is no longer valid. | |
217–218 | "made a pair" => "a Module" | |
222 | This could be changed to an assert and then the llvm_unreachable can be removed. | |
280 | Again, assert this and remove llvm_unreachable. | |
323 | Asert and remove llvm_unreachable. | |
362 | Assert and remove the llvm_unreachable. The basic form of this function appears 4 or 5 times...would it make sense to have a function taking lambdas for the varying actions (in this case, calling printIR) to common up the unpacking logic associated with Any? |
I'm not sure that having a helper method with 4 lambdas is any cleaner. Normal control flow in a function is easier to read and understand, and not really that much more code.
llvm/lib/Passes/StandardInstrumentations.cpp | ||
---|---|---|
222 | I kinda prefer the symmetry. |
Remove second sentence of this comment. It is no longer valid.