This is an archive of the discontinued LLVM Phabricator instance.

[utils] Refactor utils/update_{,llc_}test_checks.py to share more code
ClosedPublic

Authored by MaskRay on Feb 1 2018, 10:25 AM.

Details

Summary

This revision refactors 1. parser 2. CHECK line adder of utils/update_{,llc_}test_checks.py
so that thir functionality can be re-used by other utility scripts (e.g. D42712)

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Feb 1 2018, 10:25 AM

These were actually deliberately split apart in the past year, so not sure what makes sense here.

asb added a comment.Feb 2 2018, 9:13 AM

These were actually deliberately split apart in the past year, so not sure what makes sense here.

Are you referring to rL305208? It seems to me that attempts to make update_test_checks also handle llc checks were abandoned, but this patch takes a different strategy: maintaining separate 'frontends' and separate logic where it makes sense, but adding common utility functions where possible.

spatel added a comment.Feb 2 2018, 9:59 AM
In D42805#996342, @asb wrote:

These were actually deliberately split apart in the past year, so not sure what makes sense here.

Are you referring to rL305208? It seems to me that attempts to make update_test_checks also handle llc checks were abandoned, but this patch takes a different strategy: maintaining separate 'frontends' and separate logic where it makes sense, but adding common utility functions where possible.

Yep - I killed the duplicate functionality because people couldn't tell which script to use when updating llc tests (and the scripts produced slightly different output, so it was causing bogus diffs).

It's entirely my fault that I didn't do this refactoring myself because I don't know python well enough. So I fully support making these more maintainable by people who actually know what they're doing. :)

One thing that might make the usage clearer - change the script name from "update_test_checks.py" to "update_opt_test_checks.py"? We would then want to update every regression test file that has the old name as part of the 'ADVERT' string, but that should be a mechanical/scriptable change? Or better - use the new script on every test file with the old string to prove that nothing is broken in this refactoring.

MaskRay added a comment.EditedFeb 2 2018, 10:36 AM

If you prefer different scripts for different purposes:

utils/update_opt_test_checks.py   input: LLVM IR  ; CHECK lines: LLVM IR
utils/update_llc_test_checks.py   input: LLVM IR  ; CHECK lines: assembly
utils/update_cc_test_checks.py    input: C/C++/ObjC/..  ; CHECK lines: assembly or LLVM IR

If you did not mind merging update_test_checks.py and update_llc_test_checks.py, we could rename and merge these scripts by their input format:

utils/update_ir_test_checks.py      input: LLVM IR  ; CHECK lines: assembly or LLVM IR
utils/update_cc_test_checks.py     input: LLVM IR  ; CHECK lines: assembly or LLVM IR

Instead of just script name on the ADVERT line, we could also put an extra option to indicate its usage e.g.:

; NOTE: Assertions have been autogenerated by utils/update_ir_test_checks.py --llc
; NOTE: Assertions have been autogenerated by utils/update_ir_test_checks.py --opt

--llc and --opt restrict the script to update ; RUN lines that use the two tools, respectively. By merging the two scripts, we can reduce more boilerplate.

I can do that in another revision (after this revision and D42712 are landed)

I know little about MIR but it seems utils/update_mir_test_checks.py is totally different.

If you prefer different scripts for different purposes:

utils/update_opt_test_checks.py   input: LLVM IR  ; CHECK lines: LLVM IR
utils/update_llc_test_checks.py   input: LLVM IR  ; CHECK lines: assembly
utils/update_cc_test_checks.py    input: C/C++/ObjC/..  ; CHECK lines: assembly or LLVM IR

If you did not mind merging update_test_checks.py and update_llc_test_checks.py, we could rename and merge these scripts by their input format:

utils/update_ir_test_checks.py      input: LLVM IR  ; CHECK lines: assembly or LLVM IR
utils/update_cc_test_checks.py     input: LLVM IR  ; CHECK lines: assembly or LLVM IR

Instead of just script name on the ADVERT line, we could also put an extra option to indicate its usage e.g.:

; NOTE: Assertions have been autogenerated by utils/update_ir_test_checks.py --llc
; NOTE: Assertions have been autogenerated by utils/update_ir_test_checks.py --opt

--llc and --opt restrict the script to update ; RUN lines that use the two tools, respectively. By merging the two scripts, we can reduce more boilerplate.

I can do that in another revision (after this revision and D42712 are landed)

One script with an option to choose the output format sounds good to me.

Note that we do have some regression tests where we check both IR and asm in one file, so make sure that case still works. Recent example is in D42607 - test/Transforms/LoopStrengthReduce/X86/macro-fuse-cmp.ll

If you prefer different scripts for different purposes:

utils/update_opt_test_checks.py   input: LLVM IR  ; CHECK lines: LLVM IR
utils/update_llc_test_checks.py   input: LLVM IR  ; CHECK lines: assembly
utils/update_cc_test_checks.py    input: C/C++/ObjC/..  ; CHECK lines: assembly or LLVM IR

If you did not mind merging update_test_checks.py and update_llc_test_checks.py, we could rename and merge these scripts by their input format:

utils/update_ir_test_checks.py      input: LLVM IR  ; CHECK lines: assembly or LLVM IR
utils/update_cc_test_checks.py     input: LLVM IR  ; CHECK lines: assembly or LLVM IR

Instead of just script name on the ADVERT line, we could also put an extra option to indicate its usage e.g.:

; NOTE: Assertions have been autogenerated by utils/update_ir_test_checks.py --llc
; NOTE: Assertions have been autogenerated by utils/update_ir_test_checks.py --opt

--llc and --opt restrict the script to update ; RUN lines that use the two tools, respectively. By merging the two scripts, we can reduce more boilerplate.

I can do that in another revision (after this revision and D42712 are landed)

One script with an option to choose the output format sounds good to me.

Note that we do have some regression tests where we check both IR and asm in one file, so make sure that case still works. Recent example is in D42607 - test/Transforms/LoopStrengthReduce/X86/macro-fuse-cmp.ll

It still works

~/Dev/llvm/utils/update_llc_test_checks.py -v --llc-binary=~/Dev/llvm/release/bin/llc test/Transforms/LoopStrengthReduce/X86/macro-fuse-cmp.ll
# No change

~/Dev/llvm/utils/update_test_checks.py -v --opt-binary=~/Dev/llvm/release/bin/opt test/Transforms/LoopStrengthReduce/X86/macro-fuse-cmp.ll
# There are expected changes. After all the ADVERT line is update_llc_test_checks.py

Friendly ping :) Is anyone against the idea to clean up these utils/update*check_tests.py scripts?

RKSimon accepted this revision.Feb 9 2018, 3:00 PM

LGTM - thanks

This revision is now accepted and ready to land.Feb 9 2018, 3:00 PM
echristo accepted this revision.Feb 9 2018, 3:10 PM
This revision was automatically updated to reflect the committed changes.
lebedev.ri added inline comments.
llvm/trunk/utils/UpdateTestChecks/common.py
64

Hi @MaskRay.

I think there is something wrong with this particular line.

If the whitespace isn't leading/trailing, removing it will change the line,
and FileCheck will no longer match this line. So, how can we even do this?

This particular line breaks utils/update_analyze_test_checks.py
Try on test/Analysis/ScalarEvolution/*mask.ll

MaskRay added inline comments.Jun 22 2018, 12:59 PM
llvm/trunk/utils/UpdateTestChecks/common.py
64

Sorry but I don't have any idea what has been going on... When I spli clean up the script... utils/update_analyze_test_checks.py did not exist then..

RKSimon added inline comments.Jun 23 2018, 3:11 AM
llvm/trunk/utils/UpdateTestChecks/common.py
64

I wouldn't be surprised if I got something wrong on the analyze script - I wrote it for cost models only so other analysis passes weren't tested. If you can create a bugzilla with a simple repro I'll take a look (but I may need guidance from people who spend more time with python that I do).

lebedev.ri added inline comments.Jun 23 2018, 3:16 AM
llvm/trunk/utils/UpdateTestChecks/common.py
64

This is the only problem. I think this line is just wrong.
How can we change the inner context of the check line,
if we don't replace it with some [[:whitespace:]] globs or something?