This is an archive of the discontinued LLVM Phabricator instance.

A new hidden option exec-on-ir-change=exe that calls exe after each time IR changes
ClosedPublic

Authored by jamieschmeiser on Sep 29 2021, 2:15 PM.

Details

Summary

A new option exec-on-ir-changed is defined that allows one to specify an
exe that is called after each pass in the opt pipeline that changes the IR.

I recently was investigating a situation where some IR was being
mis-optimized in the IR pipeline and I needed to determine after
which pass the IR was no longer producing the correct results.
In order to determine this, I created the following option which calls
to an external exe (or script) each time the IR changes in the opt pipeline.
In my case, the exe was a script that called llc to compile the IR and ran
the result so that I could determine when it became incorrect. This is
a generally useful technique for debugging optimization bugs.

The exec-on-ir-change=exe option saves the IR in a temporary file and calls exe
with the name of the file and the name of the pass that just changed it after
each pass alters the IR. exe is also called with the initial IR. This
can be used, for example, to determine which pass corrupts the IR by having
exe as a script that calls llc and runs a test to see after which pass the
results change. The print-changed filtering options are respected.

Diff Detail

Event Timeline

jamieschmeiser created this revision.Sep 29 2021, 2:15 PM
jamieschmeiser requested review of this revision.Sep 29 2021, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2021, 2:15 PM

super nit, the title doesn't need to specify that the option is "hidden", that is definitely not important enough to be in the title

I think this could be useful, although I believe opt-bisect would also work for your use case

llvm/lib/Passes/StandardInstrumentations.cpp
73

This is way too simple of a function to use templates. They're very hard to read and are bad for compile times.
I'd say to avoid C arrays and use SmallVector/ArrayRef whenever possible.

Respond to review comments: remove templates in functions for temporary file creation and cleanup.

looks mostly good

llvm/lib/Passes/StandardInstrumentations.cpp
104

these should probably be returning llvm::Error instead of std::string

jamieschmeiser added inline comments.Dec 6 2021, 12:13 PM
llvm/lib/Passes/StandardInstrumentations.cpp
104

Is llvm::Error applicable? This isn't an error in user code but rather a failure in debug output and the error text returned is just reported as debug information.

aeubanks accepted this revision.Dec 7 2021, 10:45 AM
aeubanks added inline comments.
llvm/lib/Passes/StandardInstrumentations.cpp
104

llvm::Error is better typing than using a empty vs non-empty string, and it enforces that you check the return value
but doesn't really matter that much

This revision is now accepted and ready to land.Dec 7 2021, 10:45 AM
This revision was landed with ongoing or failed builds.Dec 8 2021, 11:23 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 8 2021, 12:16 PM

Looks like this breaks tests on Windows: http://45.33.8.238/win/50453/step_11.txt

Please take a look, and revert for now if it takes a while to fix.

I've reverted this for now. We probably shouldn't be using /bin/sh in tests. $ rg /bin/sh llvm/test shows that it's not really a thing

jamieschmeiser reopened this revision.Dec 9 2021, 7:02 AM

@aeubanks Thanks for reverting it. I'll take a look.

This revision is now accepted and ready to land.Dec 9 2021, 7:02 AM
jamieschmeiser updated this revision to Diff 393199.EditedDec 9 2021, 9:23 AM

Replace the shell script with just calling cat directly. cat is used in many lit tests and will cat the IR file and will give an error that no file exists with the name of the pass and this can be checked. @thakis, can you please verify that this does not break the windows build?

aeubanks accepted this revision.Dec 9 2021, 9:58 AM
jrtc27 added a subscriber: jrtc27.Dec 13 2021, 8:37 AM

Why not just use bugpoint? It'll bisect the pipeline and the IR itself to give you the reduced IR+misbehaving opt pass.

Yes, bugpoint will find a misbehaving class but that is just one example usage for this. test-changed is more general in that any script/exe can be called each time the IR changes. Consider the situation where a performance degradation is noticed so the code still compiles but is not as well optimized. bugpoint (as I understand from the documation) will not detect this because the IR still compiles and the exe still runs. Using test-changed and a script that calls llc followed by a timed execution, one could determine how much impact the various passes have on the performance of the code as it gets changed in the pipeline. This could point out the source(s) of speed-ups and slow-downs in the pipeline and could be used as an exploration tool.

You can use a custom run script that does whatever you like with bugpoint

bugpoint doesn't work with the new pass manager so it's effectively deprecated

jamieschmeiser retitled this revision from A new hidden option test-changed=exe that calls exe after each time IR changes to A new hidden option exec-on-ir-change=exe that calls exe after each time IR changes.
jamieschmeiser edited the summary of this revision. (Show Details)

renamed option to -exec-on-ir-change

It was suggested by @reames that -exec-on-ir-change=exe is a better name for the option. This sounds like a good suggestion to me so I have renamed it.

This revision was landed with ongoing or failed builds.Dec 16 2021, 6:02 AM
This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Dec 16 2021, 7:56 AM
Matt added a subscriber: Matt.Oct 27 2022, 9:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 9:16 AM

@aeubanks When I first reverted this because it failed on windows, I wasn't sure what to do to fix it and it fell off my radar.
I have updated it to reflect changes since then (the tests were updated to reflect changes to specifying filtering, functions have moved, etc). I've made a few minor changes, such as the functions for managing the temp files return Optional strings now and the tempfile name is different to reflect the multiple usage. I've also added a lit.local.cfg file to ensure that '/bin/cat' exists, which should fix the original windows failure that caused the revert. Can you please take a look to see that everything is still okay? Thanks.

one coding comment

llvm/lib/IR/PrintPasses.cpp
159–177

this (and below) should use ErrorOr

ErrorOr isn't really appropriate here because the string was produced when the error occurred and it was None otherwise, which is the opposite of what happens with ErrorOr. It isn't desirable to create the array of filenames using ErrorOr since you want to reuse the existing ones or a large trace will open too many temporary files. However, your comment does point out that this return is inconsistent and awkward. I've changed these functions to return an std::error_code, which is more consistent. It loses the different error strings previously produced, but this code is only used for debugging anyway. Also, I changed the function for creating the temporary files to clean up any it succeeded in creating if an error occurs.

This revision was landed with ongoing or failed builds.Dec 5 2022, 11:11 AM
This revision was automatically updated to reflect the committed changes.