This is an archive of the discontinued LLVM Phabricator instance.

Differential D48276

[llvm-mca] Stricter checking from update_mca_test_checks.py
ClosedPublic

Authored by gbedwell on Jun 18 2018, 5:11 AM.

Details

Reviewers
andreadb
RKSimon
lebedev.ri
Commits
rGbecbbe038320: [utils] Stricter checking from update_mca_test_checks.py
rL343332: [utils] Stricter checking from update_mca_test_checks.py
Summary

If any prefixes have been specified on the RUN lines that do not end up
ever actually getting printed, raise an Error. This is either an
indication that the run lines just need cleaning up, or that something
is more fundamentally wrong with the test.

Also raise an Error if there are any blocks which cannot be checked
because they are not uniquely covered by a prefix.

Ran "py update_mca_test_checks.py ..\test\tools\llvm-mca\*\*.s ..\test\tools\llvm-mca\*\*\*.s" to regenerate all tests and fixed-up a few which the new errors found issues with.

Diff Detail

Repository
rL LLVM

Event Timeline

gbedwell created this revision.Jun 18 2018, 5:11 AM
Herald added a subscriber: tschuett. · View Herald TranscriptJun 18 2018, 5:11 AM
lebedev.ri added a comment.Jun 18 2018, 5:17 AM

I did not review this yet, but it is Nice!
The other utils really need to improve in this regard (PR37752)

gbedwell added inline comments.Jun 18 2018, 5:35 AM
utils/update_mca_test_checks.py
446 ↗(On Diff #151696)

I spotted a minor indentation issue here. I'll fix before committing.

lebedev.ri added a comment.Jun 18 2018, 6:22 AM

It's a bit hard to review, the patch misses context (-U99999).

gbedwell added a comment.EditedJun 18 2018, 6:37 AM
In D48276#1135165, @lebedev.ri wrote:

It's a bit hard to review, the patch misses context (-U99999).

Good point. I lost the context when I was creating diffs for raising PR37834. I'll re-upload.

gbedwell updated this revision to Diff 151707.Jun 18 2018, 6:53 AM
gbedwell marked an inline comment as done.
gbedwell added a subscriber: llvm-commits.Jun 19 2018, 9:28 AM

Forgot to add llvm-commits

gbedwell mentioned this in D48079: [X86] Add sched class WriteLAHFSAHF and fix values..Jun 19 2018, 9:32 AM
lebedev.ri accepted this revision.Jun 25 2018, 11:50 AM

I think this makes sense.
It's not really feasible to meaningfully review these utility scripts, since there are no tests,
but phabricator does not like unaccepted differentials, so other than a few minor nits, LG.

utils/update_mca_test_checks.py
324–326 ↗(On Diff #151707)

I'm not sure, is this related to the rest of the diff?

434 ↗(On Diff #151707)

Why update? The return isn't used.
Could you just do used_prefixes |= ?
Would be more readable, too.

This revision is now accepted and ready to land.Jun 25 2018, 11:50 AM
RKSimon added a comment.Sep 10 2018, 2:05 AM

@gbedwell What is the plan for this patch?

gbedwell updated this revision to Diff 166937.Sep 25 2018, 9:47 AM

Rebased.

gbedwell marked an inline comment as done.Sep 25 2018, 9:52 AM
gbedwell added inline comments.
utils/update_mca_test_checks.py
324–326 ↗(On Diff #151707)

This is part of the same stricter checking. It, for example, caught the issue in test/tools/llvm-mca/X86/option-all-stats-1.s where the following RUN commands were generating slightly different output, but without any differentiating check prefixes.

# RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2 -all-stats=false < %s | FileCheck %s -check-prefix=ALL
# RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2                  < %s | FileCheck %s -check-prefix=ALL
gbedwell added a comment.Sep 26 2018, 4:21 AM

I've chatted with @andreadb and @RKSimon offline a bit. It would be nice to try and avoid some of the obviously duplicated blocks in the tests. For example, the added blocks in option-all-stats-1.s above. That's not actually an issue related to this specific patch, and the generated tests are still valid, but sometimes look a bit messy.

I don't want to hold this patch up based on that, but on the other hand if I can come up with something quickly, it might make sense to land them at the same time to prevent too much churn. I'll update when I know more.

gbedwell added a parent revision: D52560: [utils] Allow better identification of matching blocks in update_mca_test_checks.py.Sep 26 2018, 9:36 AM
gbedwell updated this revision to Diff 167157.Sep 26 2018, 9:38 AM

Rebase on top of D52560 which improves the block analysis significantly.

andreadb accepted this revision.Sep 26 2018, 9:54 AM

Thanks Greg.

It looks good to me.

Closed by commit rL343332: [utils] Stricter checking from update_mca_test_checks.py (authored by gbedwell). · Explain WhySep 28 2018, 8:40 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
test/
tools/
llvm-mca/
X86/
2 lines
62 lines
utils/
32 lines

Diff 167488

llvm/trunk/test/tools/llvm-mca/X86/option-all-views-2.s

# NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py # NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py
# RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2 -all-views < %s | FileCheck %s -check-prefix=ALL -check-prefix=FULLREPORT # RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2 -all-views < %s | FileCheck %s -check-prefix=ALL -check-prefix=FULLREPORT
# RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2 -all-views -resource-pressure < %s | FileCheck %s -check-prefix=ALL -check-prefix=FULLREPORT # RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2 -all-views -resource-pressure < %s | FileCheck %s -check-prefix=ALL -check-prefix=FULLREPORT
# RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2 -resource-pressure -all-views < %s | FileCheck %s -check-prefix=ALL -check-prefix=FULLREPORT # RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2 -resource-pressure -all-views < %s | FileCheck %s -check-prefix=ALL -check-prefix=FULLREPORT
# RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2 -resource-pressure=false -all-views < %s | FileCheck %s -check-prefix=ALL -check-prefix=FULLREPORT # RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2 -resource-pressure=false -all-views < %s | FileCheck %s -check-prefix=ALL -check-prefix=FULLREPORT
# RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2 -all-views -resource-pressure=false < %s | FileCheck %s -check-prefix=ALL -check-prefix=NORPV # RUN: llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2 -all-views -resource-pressure=false < %s | FileCheck %s -check-prefix=ALL
add %eax, %eax add %eax, %eax
# ALL: Iterations: 100 # ALL: Iterations: 100
# ALL-NEXT: Instructions: 100 # ALL-NEXT: Instructions: 100
# ALL-NEXT: Total Cycles: 103 # ALL-NEXT: Total Cycles: 103
# ALL-NEXT: Total uOps: 100 # ALL-NEXT: Total uOps: 100
▲ Show 20 Lines • Show All 112 Lines • Show Last 20 Lines

llvm/trunk/test/tools/llvm-mca/X86/register-file-statistics.s

# NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py # NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py
# RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=btver2 -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,BTVER2 %s # RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=btver2 -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,BTVER2 %s
# RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=znver1 -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,ZNVER1 %s # RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=znver1 -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,ZNVER1 %s
# RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=sandybridge -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,SNB %s # RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=sandybridge -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL %s
# RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=ivybridge -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,IVB %s # RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=ivybridge -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL %s
# RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=haswell -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,HSW %s # RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=haswell -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL %s
# RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=broadwell -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,BDW %s # RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=broadwell -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL %s
# RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=knl -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,KNL %s # RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=knl -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL %s
# RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=skylake -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,SKX %s # RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=skylake -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL %s
# RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=skylake-avx512 -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,SKX-AVX512 %s # RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=skylake-avx512 -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL %s
# RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=slm -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL,SLM %s # RUN: llvm-mca %s -mtriple=x86_64-unknown-unknown -mcpu=slm -iterations=1 -all-stats=false -all-views=false -register-file-stats < %s | FileCheck --check-prefixes=ALL %s
xor %eax, %ebx xor %eax, %ebx
# ALL: Register File statistics: # ALL: Register File statistics:
# ALL-NEXT: Total number of mappings created: 2 # ALL-NEXT: Total number of mappings created: 2
# ALL-NEXT: Max number of mappings used: 2 # ALL-NEXT: Max number of mappings used: 2
# BTVER2: * Register File #1 -- JFpuPRF: # BTVER2: * Register File #1 -- JFpuPRF:
# BTVER2-NEXT: Number of physical registers: 72 # BTVER2-NEXT: Number of physical registers: 72
# BTVER2-NEXT: Total number of mappings created: 0 # BTVER2-NEXT: Total number of mappings created: 0
# BTVER2-NEXT: Max number of mappings used: 0 # BTVER2-NEXT: Max number of mappings used: 0
# ZNVER1: * Register File #1 -- ZnFpuPRF: # ZNVER1: * Register File #1 -- ZnFpuPRF:
# ZNVER1-NEXT: Number of physical registers: 160 # ZNVER1-NEXT: Number of physical registers: 160
# ZNVER1-NEXT: Total number of mappings created: 0 # ZNVER1-NEXT: Total number of mappings created: 0
# ZNVER1-NEXT: Max number of mappings used: 0 # ZNVER1-NEXT: Max number of mappings used: 0
# BTVER2: * Register File #2 -- JIntegerPRF: # BTVER2: * Register File #2 -- JIntegerPRF:
# BTVER2-NEXT: Number of physical registers: 64 # BTVER2-NEXT: Number of physical registers: 64
# BTVER2-NEXT: Total number of mappings created: 2 # BTVER2-NEXT: Total number of mappings created: 2
# BTVER2-NEXT: Max number of mappings used: 2 # BTVER2-NEXT: Max number of mappings used: 2
# ZNVER1: * Register File #2 -- ZnIntegerPRF: # ZNVER1: * Register File #2 -- ZnIntegerPRF:
# ZNVER1-NEXT: Number of physical registers: 168 # ZNVER1-NEXT: Number of physical registers: 168
# ZNVER1-NEXT: Total number of mappings created: 2 # ZNVER1-NEXT: Total number of mappings created: 2
# ZNVER1-NEXT: Max number of mappings used: 2 # ZNVER1-NEXT: Max number of mappings used: 2

llvm/trunk/utils/update_mca_test_checks.py

Show First 20 Lines • Show All 382 Lines • ▼ Show 20 Lines for i, block_text in enumerate(block_infos[block_num]):
for s in pruned_sets[i]: for s in pruned_sets[i]:
s = sorted(list(s)) s = sorted(list(s))
if s: if s:
current_set.add(s[0]) current_set.add(s[0])
if len(s) > 1: if len(s) > 1:
_warn('Multiple prefixes generating same output: {} ' _warn('Multiple prefixes generating same output: {} '
'(discarding {})'.format(','.join(s), ','.join(s[1:]))) '(discarding {})'.format(','.join(s), ','.join(s[1:])))
if block_text and not current_set:
raise Error(
'block not captured by existing prefixes:\n\n{}'.format(block_text))
block_infos[block_num][block_text] = sorted(list(current_set)) block_infos[block_num][block_text] = sorted(list(current_set))
# If we have multiple block_texts, try to break them down further to avoid # If we have multiple block_texts, try to break them down further to avoid
# the case where we have very similar block_texts repeated after each # the case where we have very similar block_texts repeated after each
# other. # other.
if common_prefix and len(block_infos[block_num]) > 1: if common_prefix and len(block_infos[block_num]) > 1:
# We'll only attempt this if each of the block_texts have the same number # We'll only attempt this if each of the block_texts have the same number
# of lines as each other. # of lines as each other.
same_num_Lines = (len(set(len(k.splitlines()) same_num_Lines = (len(set(len(k.splitlines())
for k in block_infos[block_num].keys())) == 1) for k in block_infos[block_num].keys())) == 1)
if same_num_Lines: if same_num_Lines:
breakdown = _break_down_block(block_infos[block_num], common_prefix) breakdown = _break_down_block(block_infos[block_num], common_prefix)
if breakdown: if breakdown:
block_infos[block_num] = breakdown block_infos[block_num] = breakdown
return block_infos return block_infos
def _write_block(output, block, not_prefix_set, common_prefix, prefix_pad): def _write_block(output, block, not_prefix_set, common_prefix, prefix_pad):
""" Write an individual block, with correct padding on the prefixes. """ Write an individual block, with correct padding on the prefixes.
Returns a set of all of the prefixes that it has written.
""" """
end_prefix = ': ' end_prefix = ': '
previous_prefix = None previous_prefix = None
num_lines_of_prefix = 0 num_lines_of_prefix = 0
written_prefixes = set()
for prefix, line in block: for prefix, line in block:
if prefix in not_prefix_set: if prefix in not_prefix_set:
_warn('not writing for prefix {0} due to presence of "{0}-NOT:" ' _warn('not writing for prefix {0} due to presence of "{0}-NOT:" '
'in input file.'.format(prefix)) 'in input file.'.format(prefix))
continue continue
# If the previous line isn't already blank and we're writing more than one # If the previous line isn't already blank and we're writing more than one
# line for the current prefix output a blank line first, unless either the # line for the current prefix output a blank line first, unless either the
# current of previous prefix is common to all. # current of previous prefix is common to all.
num_lines_of_prefix += 1 num_lines_of_prefix += 1
if prefix != previous_prefix: if prefix != previous_prefix:
if output and output[-1]: if output and output[-1]:
if num_lines_of_prefix > 1 or any(p == common_prefix if num_lines_of_prefix > 1 or any(p == common_prefix
for p in (prefix, previous_prefix)): for p in (prefix, previous_prefix)):
output.append('') output.append('')
num_lines_of_prefix = 0 num_lines_of_prefix = 0
previous_prefix = prefix previous_prefix = prefix
written_prefixes.add(prefix)
output.append( output.append(
'{} {}{}{} {}'.format(COMMENT_CHAR, '{} {}{}{} {}'.format(COMMENT_CHAR,
prefix, prefix,
end_prefix, end_prefix,
' ' * (prefix_pad - len(prefix)), ' ' * (prefix_pad - len(prefix)),
line).rstrip()) line).rstrip())
end_prefix = '-NEXT:' end_prefix = '-NEXT:'
output.append('') output.append('')
return written_prefixes
def _write_output(test_path, input_lines, prefix_list, block_infos, # noqa def _write_output(test_path, input_lines, prefix_list, block_infos, # noqa
args, common_prefix, prefix_pad): args, common_prefix, prefix_pad):
prefix_set = set([prefix for prefixes, _ in prefix_list prefix_set = set([prefix for prefixes, _ in prefix_list
for prefix in prefixes]) for prefix in prefixes])
not_prefix_set = set() not_prefix_set = set()
Show All 27 Lines for input_line in input_lines:
else: else:
continue continue
# Add a blank line before the new checks if required. # Add a blank line before the new checks if required.
if len(output_lines) > 0 and output_lines[-1]: if len(output_lines) > 0 and output_lines[-1]:
output_lines.append('') output_lines.append('')
output_check_lines = [] output_check_lines = []
used_prefixes = set()
for block_num in range(len(block_infos)): for block_num in range(len(block_infos)):
for block_text in sorted(block_infos[block_num]): for block_text in sorted(block_infos[block_num]):
if not block_text: if not block_text:
continue continue
if type(block_infos[block_num]) is list: if type(block_infos[block_num]) is list:
# The block is of the type output from _break_down_block(). # The block is of the type output from _break_down_block().
_write_block(output_check_lines, used_prefixes |= _write_block(output_check_lines,
block_infos[block_num], block_infos[block_num],
not_prefix_set, not_prefix_set,
common_prefix, common_prefix,
prefix_pad) prefix_pad)
break break
elif block_infos[block_num][block_text]: elif block_infos[block_num][block_text]:
# _break_down_block() was unable to do do anything so output the block # _break_down_block() was unable to do do anything so output the block
# as-is. # as-is.
lines = block_text.split('\n') lines = block_text.split('\n')
for prefix in block_infos[block_num][block_text]: for prefix in block_infos[block_num][block_text]:
_write_block(output_check_lines, used_prefixes |= _write_block(output_check_lines,
[(prefix, line) for line in lines], [(prefix, line) for line in lines],
not_prefix_set, not_prefix_set,
common_prefix, common_prefix,
prefix_pad) prefix_pad)
unused_prefixes = (prefix_set - not_prefix_set) - used_prefixes
if unused_prefixes:
raise Error('unused prefixes: {}'.format(sorted(unused_prefixes)))
if output_check_lines: if output_check_lines:
output_lines.insert(0, ADVERT) output_lines.insert(0, ADVERT)
output_lines.extend(output_check_lines) output_lines.extend(output_check_lines)
# The file should not end with two newlines. It creates unnecessary churn. # The file should not end with two newlines. It creates unnecessary churn.
while len(output_lines) > 0 and output_lines[-1] == '': while len(output_lines) > 0 and output_lines[-1] == '':
output_lines.pop() output_lines.pop()
▲ Show 20 Lines • Show All 55 Lines • Show Last 20 Lines