This is an archive of the discontinued LLVM Phabricator instance.

[UpdateLLCTestChecks] Add support for isel debug output in update_llc_test_checks.py
ClosedPublic

Authored by ningxinr on Feb 9 2022, 12:44 PM.

Details

Summary

Add a check on run lines to pick up isel options in llc commands and allow
generating check lines of isel final output other than assembly. If llc command
line contains -debug-only=isel, update_llc_test_checks.py will try to scrub isel
output, otherwise, the script will fall back on default behaviour, which is try to
scrub assembly output instead.

The motivation of this change is to allow usage of update_llc_test_checks.py to
autogenerate checks of instruction selection results. In this way, we can detect
errors at an earlier stage before the compilation goes all the way to assembly.
It is an example of having some transparency for the stages between IR and
assembly. These generated tests are almost like "unit tests" of isel stage.

This patch only implements the initial change to differentiate isel output from
assembly output for Lanai. Other targets will not be supported for isel check
generation at the moment. Although adding support for it will only require
implementing the function regex and scrubber for corresponding targets.

The Lanai implementation was chosen mainly for the simplicity of demonstrating
the difference between isel checks and asm checks.

This patch also do not include the implementation of function prefix, which is
required for the generated isel checks to pass. I will put up a follow up revision
for the function prefix change to complete isel support.

Diff Detail

Event Timeline

ningxinr created this revision.Feb 9 2022, 12:44 PM
ningxinr requested review of this revision.Feb 9 2022, 12:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2022, 12:44 PM
ningxinr edited the summary of this revision. (Show Details)Feb 9 2022, 1:14 PM
ningxinr edited the summary of this revision. (Show Details)Feb 9 2022, 1:19 PM
Flakebi added inline comments.Feb 14 2022, 1:47 AM
llvm/utils/UpdateTestChecks/isel.py
34–41

Does it make sense to put get_triple_from_march into common.py or update_llc_test_checks.py? It doesn’t look isel or asm specific.

ningxinr updated this revision to Diff 408599.Feb 14 2022, 1:53 PM

Moved get_triple_from_march into common.py per suggested by Sebastian.

ningxinr added inline comments.Feb 14 2022, 1:57 PM
llvm/utils/UpdateTestChecks/isel.py
34–41

Yes. I have moved get_triple_from_march into common.py.

Sorry for taking so long to review this.

I only needed to slightly adjust the regex and it worked fine for amdgpu and x86, so what do you think about returning a default from get_run_handler when no target-specific override is found?

The adjusted start regex is
r'Selected[\s]*selection[\s]*DAG:[\s]*%bb.0[\s]*\'(?P<func>.*?):[^\']*\'*\n'
with a [^\']* after the :. (for amdgpu, an example line is Selected selection DAG: %bb.0 'test1:main_body', for x86 e.g. Selected selection DAG: %bb.0 'foo:entry')

ningxinr updated this revision to Diff 411210.Feb 24 2022, 12:20 PM

Updated the default handler per suggestion by Sebastian

Sorry for taking so long to review this.

I only needed to slightly adjust the regex and it worked fine for amdgpu and x86, so what do you think about returning a default from get_run_handler when no target-specific override is found?

The adjusted start regex is
r'Selected[\s]*selection[\s]*DAG:[\s]*%bb.0[\s]*\'(?P<func>.*?):[^\']*\'*\n'
with a [^\']* after the :. (for amdgpu, an example line is Selected selection DAG: %bb.0 'test1:main_body', for x86 e.g. Selected selection DAG: %bb.0 'foo:entry')

Does the latest version looks like something you had in mind, Sebastian? :)

Flakebi accepted this revision.Feb 25 2022, 12:55 AM

Yes, looks great, thanks

This revision is now accepted and ready to land.Feb 25 2022, 12:55 AM

Yes, looks great, thanks

Thank you Sebastian!

Anyone can help me commit this change? I don't have the permission myself. Thank you thank you!

sebastian-ne added inline comments.
llvm/test/tools/UpdateTestChecks/update_llc_test_checks/isel-support.test
1 ↗(On Diff #411210)

I think this file needs to be split into 3 different files, each having a

# REQUIRES: aarch64-registered-target

for the respective target at the start.
Otherwise the tests will fail when the respective target is not built.

llvm/utils/UpdateTestChecks/isel.py
36

nit: There is one extra space here

ningxinr updated this revision to Diff 411893.Feb 28 2022, 2:05 PM

Refactored based on feedback: remove the excessive space and split isle-support.test based on architecture

ningxinr marked 2 inline comments as done.Feb 28 2022, 2:07 PM

Marked done items.

This revision was landed with ongoing or failed builds.Mar 1 2022, 2:00 AM
This revision was automatically updated to reflect the committed changes.

Thank you for helping me committing this, Sebastian! :)

No problem.

Some buildbot failures were caused by this because -debug-only is not available in release mode. I added REQUIRES: asserts in c74f54f2f4515b6857067ef893fc4d73b14b373e, I hope that fixes it.