Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[llvm-mca] Stricter checking from

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



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 ..\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


Event Timeline

gbedwell created this revision.Jun 18 2018, 5:11 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
446 ↗(On Diff #151696)

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

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

gbedwell added a comment.EditedJun 18 2018, 6:37 AM

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.

Forgot to add llvm-commits

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.

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

@gbedwell What is the plan for this patch?

gbedwell marked an inline comment as done.Sep 25 2018, 9:52 AM
gbedwell added inline comments.
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

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 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.

This revision was automatically updated to reflect the committed changes.