Page MenuHomePhabricator

[UpdateTestChecks] Update tests option
ClosedPublic

Authored by xbolva00 on Aug 6 2019, 2:05 AM.

Details

Summary

Port of new feature introduced https://reviews.llvm.org/D65610 to other update scripts.

  • update_*_checks.py: add an alias -u for --update-only
  • port --update-only to other update_*_test_checks.py scripts
  • update script aborts if the test file was generated by another update_*_test_checks.py utility

Diff Detail

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.Aug 6 2019, 2:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 2:05 AM

Testing LLC updater now.. looks fine.

Use case: ./update_llc_test_checks.py -u ../test/CodeGen/X86/*.ll

xbolva00 updated this revision to Diff 213547.Aug 6 2019, 2:11 AM
xbolva00 retitled this revision from [UpdateTestChecks] Emit warning when invalid test paths to [UpdateTestChecks] Update tests option.Aug 6 2019, 2:37 AM
MaskRay added inline comments.Aug 6 2019, 2:51 AM
utils/UpdateTestChecks/common.py
76 ↗(On Diff #213547)

This file is indented by 2.

297 ↗(On Diff #213547)

I don't know if a common function is useful, ideally an update*.py utility should abort if the test file was generated by another update*.py utility, e.g.

update_llc_test_checks.py should abort if the file was generated by update_test_checks.py.

utils/update_analyze_test_checks.py
102 ↗(On Diff #213547)

Use + to concatenate two strings.

utils/update_cc_test_checks.py
139 ↗(On Diff #213547)

Use + to concatenate two strings.

162 ↗(On Diff #213547)

If it is clear that l in '... %s' % (l,) is a str, the pattern should be converted to

'... ' + l for clarity.

%s is not printf C string, it is str() applied on the expression... (So '%s' % ([1,2,3],) works)

utils/update_llc_test_checks.py
84 ↗(On Diff #213547)

Use + to concatenate two strings.

utils/update_mir_test_checks.py
117 ↗(On Diff #213547)

Use + to concatenate two strings.

xbolva00 updated this revision to Diff 213709.Aug 6 2019, 1:52 PM
xbolva00 marked an inline comment as done.

Addressed review notes.

xbolva00 marked 7 inline comments as done.Aug 6 2019, 1:53 PM
xbolva00 added inline comments.
utils/UpdateTestChecks/common.py
297 ↗(On Diff #213547)

Yes, such checker would be useful addition. I will do it.

xbolva00 marked 2 inline comments as done.Aug 6 2019, 1:53 PM
xbolva00 added inline comments.
utils/UpdateTestChecks/common.py
297 ↗(On Diff #213547)

Done.

MaskRay accepted this revision.Aug 6 2019, 7:20 PM

The patch does 3 things now:

  • update_test_checks.py: add an alias -u for --update-only
  • port --update-only to other update_*_test_checks.py scripts
  • the script aborts if the test file was generated by another update_*_test_checks.py utility

The description should probably reflect the situation.

This revision is now accepted and ready to land.Aug 6 2019, 7:20 PM
xbolva00 edited the summary of this revision. (Show Details)Aug 7 2019, 7:42 AM
This revision was automatically updated to reflect the committed changes.
lkail added a subscriber: lkail.Dec 2 2019, 11:57 PM
lkail added inline comments.
llvm/trunk/utils/update_analyze_test_checks.py
107

Hi @xbolva00 , I don't understand why skipping lines without '|'. There are still many autogenerated test cases in backend written kinda like

; RUN: llc ... \
; RUN: | FileCheck ...
lkail added a comment.Dec 3 2019, 12:50 AM
This comment was removed by lkail.
lkail marked an inline comment as done.Dec 3 2019, 12:51 AM
lkail added inline comments.
llvm/trunk/utils/update_analyze_test_checks.py
107

Sorry for disturbing you, https://reviews.llvm.org/D70941 should have solved my concern.