This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Cleanup IR printing instrumentation
ClosedPublic

Authored by aeubanks on Apr 9 2021, 2:22 PM.

Details

Summary
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.

Diff Detail

Event Timeline

aeubanks created this revision.Apr 9 2021, 2:22 PM
aeubanks requested review of this revision.Apr 9 2021, 2:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2021, 2:22 PM
aeubanks updated this revision to Diff 336952.Apr 12 2021, 1:43 PM

rebase, major overhaul

aeubanks edited the summary of this revision. (Show Details)Apr 12 2021, 1:44 PM
aeubanks updated this revision to Diff 336953.Apr 12 2021, 1:58 PM

more changes

aeubanks updated this revision to Diff 336971.Apr 12 2021, 3:03 PM

test fixes

jamieschmeiser requested changes to this revision.Apr 13 2021, 7:25 AM

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?

This revision now requires changes to proceed.Apr 13 2021, 7:25 AM
aeubanks updated this revision to Diff 337572.Apr 14 2021, 3:14 PM
aeubanks marked 2 inline comments as done.

address comments

Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2021, 3:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

This revision is now accepted and ready to land.Apr 15 2021, 5:47 AM
This revision was landed with ongoing or failed builds.Apr 15 2021, 9:51 AM
This revision was automatically updated to reflect the committed changes.