This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Add callbacks to PassBuilder to run before/after parsing a pass
ClosedPublic

Authored by aeubanks on Sep 22 2020, 11:06 AM.

Details

Summary

This is in preparation for supporting -debugify-each, which adds a debug
info pass before and after each pass.

Switch VerifyEach to use this.

Diff Detail

Event Timeline

aeubanks created this revision.Sep 22 2020, 11:06 AM
Herald added a project: Restricted Project. · View Herald Transcript
aeubanks requested review of this revision.Sep 22 2020, 11:06 AM
aeubanks updated this revision to Diff 293512.Sep 22 2020, 11:08 AM

update comment
rename

aeubanks updated this revision to Diff 293520.Sep 22 2020, 11:12 AM

small comment change

ychen added inline comments.Sep 22 2020, 1:20 PM
llvm/include/llvm/Passes/PassBuilder.h
828

How about adding a "StringRef Element" as the first callback parameter? I could not think of a concrete use case for it now. But it is more general and canonical. The current interface is restricted to callbacks that do something for every pass.

For the name, why not just use before/after instead of willParse/parsed?

llvm/unittests/IR/PassBuilderCallbacksTest.cpp
1189

assert (I==0); ?

aeubanks added inline comments.Sep 22 2020, 5:24 PM
llvm/include/llvm/Passes/PassBuilder.h
828

We can add StringRef Element in a later patch if a need arises, I would prefer not to add too much now.

I was trying to think of a nice name but couldn't think of that for some reason :). Renamed.

llvm/unittests/IR/PassBuilderCallbacksTest.cpp
1189

The CounterPasses added in the callbacks will run multiple times since these also run on implicitly created PassManagers, and I only want to catch very first time one runs.

ychen accepted this revision.Sep 22 2020, 6:02 PM

LGTM. Thanks.

llvm/unittests/IR/PassBuilderCallbacksTest.cpp
1189

I see. Could you please add a comment about this?

This revision is now accepted and ready to land.Sep 22 2020, 6:02 PM
aeubanks updated this revision to Diff 293611.Sep 22 2020, 6:08 PM

add comment

This revision was landed with ongoing or failed builds.Sep 23 2020, 3:28 PM
This revision was automatically updated to reflect the committed changes.

Turns out this doesn't actually work as intended...
The callbacks need to be done at the addPass() level, or else something like 'default<O3>' only runs the callbacks once, even though it adds a bunch of passes. Will probably revert once I come up with something better.

Actually, doing verify-each in StandardInstrumentations seems much nicer. Not sure about how to do debugify-each though, filed https://bugs.llvm.org/show_bug.cgi?id=47633.

ychen added a comment.Sep 23 2020, 6:59 PM

Actually, doing verify-each in StandardInstrumentations seems much nicer. Not sure about how to do debugify-each though, filed https://bugs.llvm.org/show_bug.cgi?id=47633.

If debugify-each has or could be made to have a standalone function interface, could it use StandardInstrumentations?

Actually, doing verify-each in StandardInstrumentations seems much nicer. Not sure about how to do debugify-each though, filed https://bugs.llvm.org/show_bug.cgi?id=47633.

If debugify-each has or could be made to have a standalone function interface, could it use StandardInstrumentations?

The issue is that it modifies the IR, which can't be done from StandardInstrumentations (invalidating analyses, etc.).
-verify doesn't modify the IR.

Actually, doing verify-each in StandardInstrumentations seems much nicer. Not sure about how to do debugify-each though, filed https://bugs.llvm.org/show_bug.cgi?id=47633.

If debugify-each has or could be made to have a standalone function interface, could it use StandardInstrumentations?

The issue is that it modifies the IR, which can't be done from StandardInstrumentations (invalidating analyses, etc.).
-verify doesn't modify the IR.

I worked on test/DebugInfo from a task list by @aeubanks and found this thread just after I posted my -debugify-each patch D90365:)
Hope it does not collide with any work you are working on:)

I have tried StandardInstrumentations but switched to moving all stuff in Debugify.cpp because that seems cleaner (I don't want things in Debugify.cpp leak to other places).