search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
25.95k stars 10.59k forks source link

[llvm-mca] Abort on parse error without -skip-unsupported-instructions #90474

Closed peterwaller-arm closed 1 week ago

peterwaller-arm commented 2 weeks ago

[llvm-mca] Abort on parse error without -skip-unsupported-instructions

Prior to this patch, llvm-mca would continue executing after parse errors. These errors can lead to some confusion since some analysis results are printed on the standard output, and they're printed after the errors, which could otherwise be easy to miss.

However it is still useful to be able to continue analysis after errors; so extend the recently added -skip-unsupported-instructions to support this.

Two tests which have parse errors for some of the 'RUN' branches are updated to use -skip-unsupported-instructions so they can remain as-is.

Add a description of -skip-unsupported-instructions to the llvm-mca command guide, and add it to the llvm-mca --help output:

  --skip-unsupported-instructions=<value> - Force analysis to continue in the presence of unsupported instructions
    =none                                 -   Exit with an error when an instruction is unsupported for any reason (default)
    =lack-sched                           -   Skip instructions on input which lack scheduling information
    =parse-failure                        -   Skip lines on the input which fail to parse for any reason
    =any                                  -   Skip instructions or lines on input which are unsupported for any reason

Tests within this patch are intended to cover each of the cases.

Reason Flag Comment
none none Usual case, existing test suite
lack-sched none Advises user to use -skip-unsupported-instructions=lack-sched, tested in llvm/test/tools/llvm-mca/X86/BtVer2/unsupported-instruction.s
parse-failure none Advises user to use -skip-unsupported-instructions=parse-failure, tested in llvm/test/tools/llvm-mca/bad-input.s
any none (N/A, covered above)
lack-sched any Continues, prints warnings, tested in llvm/test/tools/llvm-mca/X86/BtVer2/unsupported-instruction.s
parse-failure any Continues, prints errors, tested in llvm/test/tools/llvm-mca/bad-input.s
lack-sched parse-failure Advises user to use -skip-unsupported-instructions=lack-sched, tested in llvm/test/tools/llvm-mca/X86/BtVer2/unsupported-instruction.s
parse-failure lack-sched Advises user to use -skip-unsupported-instructions=parse-failure, tested in llvm/test/tools/llvm-mca/bad-input.s
none * This would be any test case with skip-unsupported-instructions, coverage added in llvm/test/tools/llvm-mca/X86/BtVer2/simple-test.s
github-actions[bot] commented 2 weeks ago

:white_check_mark: With the latest revision this PR passed the C/C++ code formatter.

llvmbot commented 2 weeks ago

@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-tools-llvm-mca

Author: Peter Waller (peterwaller-arm)

Changes Prior to this patch, llvm-mca would continue executing after parse errors. These errors can lead to some confusion since some analysis results are printed on the standard output, and they're printed after the errors, which could otherwise be easy to miss. However it is still useful to be able to continue analysis after errors; so extend the recently added -skip-unsupported-instructions to support this. Two tests which have parse errors for some of the 'RUN' branches are updated to use -skip-unsupported-instructions so they can remain as-is. --- Full diff: https://github.com/llvm/llvm-project/pull/90474.diff 6 Files Affected: - (modified) llvm/test/tools/llvm-mca/AArch64/Exynos/float-divide-multiply.s (+1-1) - (modified) llvm/test/tools/llvm-mca/AArch64/Exynos/float-integer.s (+1-1) - (added) llvm/test/tools/llvm-mca/bad-input.s (+11) - (modified) llvm/tools/llvm-mca/CodeRegionGenerator.cpp (+5-2) - (modified) llvm/tools/llvm-mca/CodeRegionGenerator.h (+24-12) - (modified) llvm/tools/llvm-mca/llvm-mca.cpp (+3-2) ``````````diff diff --git a/llvm/test/tools/llvm-mca/AArch64/Exynos/float-divide-multiply.s b/llvm/test/tools/llvm-mca/AArch64/Exynos/float-divide-multiply.s index ecfd019452afcd..b726933f1b5573 100644 --- a/llvm/test/tools/llvm-mca/AArch64/Exynos/float-divide-multiply.s +++ b/llvm/test/tools/llvm-mca/AArch64/Exynos/float-divide-multiply.s @@ -1,5 +1,5 @@ # NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py -# RUN: llvm-mca -march=aarch64 -mcpu=exynos-m3 -resource-pressure=false < %s | FileCheck %s -check-prefixes=ALL,EM3 +# RUN: llvm-mca -march=aarch64 -mcpu=exynos-m3 -resource-pressure=false -skip-unsupported-instructions < %s | FileCheck %s -check-prefixes=ALL,EM3 # RUN: llvm-mca -march=aarch64 -mcpu=exynos-m4 -resource-pressure=false < %s | FileCheck %s -check-prefixes=ALL,EM4 # RUN: llvm-mca -march=aarch64 -mcpu=exynos-m5 -resource-pressure=false < %s | FileCheck %s -check-prefixes=ALL,EM5 diff --git a/llvm/test/tools/llvm-mca/AArch64/Exynos/float-integer.s b/llvm/test/tools/llvm-mca/AArch64/Exynos/float-integer.s index 16c710553f754a..46baecead72bee 100644 --- a/llvm/test/tools/llvm-mca/AArch64/Exynos/float-integer.s +++ b/llvm/test/tools/llvm-mca/AArch64/Exynos/float-integer.s @@ -1,5 +1,5 @@ # NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py -# RUN: llvm-mca -mtriple=aarch64-linux-gnu -mcpu=exynos-m3 -resource-pressure=false < %s | FileCheck %s -check-prefixes=ALL,EM3 +# RUN: llvm-mca -mtriple=aarch64-linux-gnu -mcpu=exynos-m3 -resource-pressure=false -skip-unsupported-instructions < %s | FileCheck %s -check-prefixes=ALL,EM3 # RUN: llvm-mca -mtriple=aarch64-linux-gnu -mcpu=exynos-m4 -resource-pressure=false < %s | FileCheck %s -check-prefixes=ALL,EM4 # RUN: llvm-mca -mtriple=aarch64-linux-gnu -mcpu=exynos-m5 -resource-pressure=false < %s | FileCheck %s -check-prefixes=ALL,EM5 diff --git a/llvm/test/tools/llvm-mca/bad-input.s b/llvm/test/tools/llvm-mca/bad-input.s new file mode 100644 index 00000000000000..a412f10394fb25 --- /dev/null +++ b/llvm/test/tools/llvm-mca/bad-input.s @@ -0,0 +1,11 @@ +# RUN: not llvm-mca %s -o /dev/null 2>&1 | FileCheck --check-prefixes=CHECK-ALL,CHECK %s +# RUN: not llvm-mca -skip-unsupported-instructions %s -o /dev/null 2>&1 | FileCheck --check-prefixes=CHECK-ALL,CHECK-SKIP %s + +# Test checks that MCA does not produce a total cycles estimate if it encounters parse errors. + +# CHECK-ALL-NOT: Total Cycles: + +# CHECK: error: Assembly input parsing had errors. +# CHECK-SKIP: error: no assembly instructions found. + +This is not a valid assembly file for any architecture (by virtue of this text.) diff --git a/llvm/tools/llvm-mca/CodeRegionGenerator.cpp b/llvm/tools/llvm-mca/CodeRegionGenerator.cpp index 5241b584b74661..77ab6584589e4c 100644 --- a/llvm/tools/llvm-mca/CodeRegionGenerator.cpp +++ b/llvm/tools/llvm-mca/CodeRegionGenerator.cpp @@ -29,7 +29,8 @@ namespace mca { CodeRegionGenerator::~CodeRegionGenerator() {} Expected AsmCodeRegionGenerator::parseCodeRegions( - const std::unique_ptr &IP) { + const std::unique_ptr &IP, + bool SkipUnsupportedInstructions) { MCTargetOptions Opts; Opts.PreserveAsmComments = false; CodeRegions &Regions = getRegions(); @@ -61,7 +62,9 @@ Expected AsmCodeRegionGenerator::parseCodeRegions( "This target does not support assembly parsing.", inconvertibleErrorCode()); Parser->setTargetParser(*TAP); - Parser->Run(false); + if (Parser->Run(false) && !SkipUnsupportedInstructions) + return make_error("Assembly input parsing had errors.", + inconvertibleErrorCode()); if (CCP->hadErr()) return make_error("There was an error parsing comments.", diff --git a/llvm/tools/llvm-mca/CodeRegionGenerator.h b/llvm/tools/llvm-mca/CodeRegionGenerator.h index 68da567f3e0f32..8fd988bf97f0a1 100644 --- a/llvm/tools/llvm-mca/CodeRegionGenerator.h +++ b/llvm/tools/llvm-mca/CodeRegionGenerator.h @@ -148,7 +148,8 @@ class CodeRegionGenerator { CodeRegionGenerator(const CodeRegionGenerator &) = delete; CodeRegionGenerator &operator=(const CodeRegionGenerator &) = delete; virtual Expected - parseCodeRegions(const std::unique_ptr &IP) = 0; + parseCodeRegions(const std::unique_ptr &IP, + bool SkipUnsupportedInstructions) = 0; public: CodeRegionGenerator() {} @@ -164,7 +165,8 @@ class AnalysisRegionGenerator : public virtual CodeRegionGenerator { AnalysisRegionGenerator(llvm::SourceMgr &SM) : Regions(SM) {} virtual Expected - parseAnalysisRegions(const std::unique_ptr &IP) = 0; + parseAnalysisRegions(const std::unique_ptr &IP, + bool SkipUnsupportedInstructions) = 0; }; /// Abstract CodeRegionGenerator with InstrumentRegionsRegions member @@ -176,7 +178,8 @@ class InstrumentRegionGenerator : public virtual CodeRegionGenerator { InstrumentRegionGenerator(llvm::SourceMgr &SM) : Regions(SM) {} virtual Expected - parseInstrumentRegions(const std::unique_ptr &IP) = 0; + parseInstrumentRegions(const std::unique_ptr &IP, + bool SkipUnsupportedInstructions) = 0; }; /// This abstract class is responsible for parsing input ASM and @@ -202,7 +205,8 @@ class AsmCodeRegionGenerator : public virtual CodeRegionGenerator { unsigned getAssemblerDialect() const { return AssemblerDialect; } Expected - parseCodeRegions(const std::unique_ptr &IP) override; + parseCodeRegions(const std::unique_ptr &IP, + bool SkipUnsupportedInstructions) override; }; class AsmAnalysisRegionGenerator final : public AnalysisRegionGenerator, @@ -222,8 +226,10 @@ class AsmAnalysisRegionGenerator final : public AnalysisRegionGenerator, MCStreamerWrapper *getMCStreamer() override { return &Streamer; } Expected - parseAnalysisRegions(const std::unique_ptr &IP) override { - Expected RegionsOrErr = parseCodeRegions(IP); + parseAnalysisRegions(const std::unique_ptr &IP, + bool SkipUnsupportedInstructions) override { + Expected RegionsOrErr = + parseCodeRegions(IP, SkipUnsupportedInstructions); if (!RegionsOrErr) return RegionsOrErr.takeError(); else @@ -231,8 +237,10 @@ class AsmAnalysisRegionGenerator final : public AnalysisRegionGenerator, } Expected - parseCodeRegions(const std::unique_ptr &IP) override { - return AsmCodeRegionGenerator::parseCodeRegions(IP); + parseCodeRegions(const std::unique_ptr &IP, + bool SkipUnsupportedInstructions) override { + return AsmCodeRegionGenerator::parseCodeRegions( + IP, SkipUnsupportedInstructions); } }; @@ -254,8 +262,10 @@ class AsmInstrumentRegionGenerator final : public InstrumentRegionGenerator, MCStreamerWrapper *getMCStreamer() override { return &Streamer; } Expected - parseInstrumentRegions(const std::unique_ptr &IP) override { - Expected RegionsOrErr = parseCodeRegions(IP); + parseInstrumentRegions(const std::unique_ptr &IP, + bool SkipUnsupportedInstructions) override { + Expected RegionsOrErr = + parseCodeRegions(IP, SkipUnsupportedInstructions); if (!RegionsOrErr) return RegionsOrErr.takeError(); else @@ -263,8 +273,10 @@ class AsmInstrumentRegionGenerator final : public InstrumentRegionGenerator, } Expected - parseCodeRegions(const std::unique_ptr &IP) override { - return AsmCodeRegionGenerator::parseCodeRegions(IP); + parseCodeRegions(const std::unique_ptr &IP, + bool SkipUnsupportedInstructions) override { + return AsmCodeRegionGenerator::parseCodeRegions( + IP, SkipUnsupportedInstructions); } }; diff --git a/llvm/tools/llvm-mca/llvm-mca.cpp b/llvm/tools/llvm-mca/llvm-mca.cpp index e037c06b12a35d..674a2da551b2c4 100644 --- a/llvm/tools/llvm-mca/llvm-mca.cpp +++ b/llvm/tools/llvm-mca/llvm-mca.cpp @@ -440,7 +440,7 @@ int main(int argc, char **argv) { mca::AsmAnalysisRegionGenerator CRG(*TheTarget, SrcMgr, ACtx, *MAI, *STI, *MCII); Expected RegionsOrErr = - CRG.parseAnalysisRegions(std::move(IPtemp)); + CRG.parseAnalysisRegions(std::move(IPtemp), SkipUnsupportedInstructions); if (!RegionsOrErr) { if (auto Err = handleErrors(RegionsOrErr.takeError(), [](const StringError &E) { @@ -482,7 +482,8 @@ int main(int argc, char **argv) { mca::AsmInstrumentRegionGenerator IRG(*TheTarget, SrcMgr, ICtx, *MAI, *STI, *MCII, *IM); Expected InstrumentRegionsOrErr = - IRG.parseInstrumentRegions(std::move(IPtemp)); + IRG.parseInstrumentRegions(std::move(IPtemp), + SkipUnsupportedInstructions); if (!InstrumentRegionsOrErr) { if (auto Err = handleErrors(InstrumentRegionsOrErr.takeError(), [](const StringError &E) { ``````````
adibiagio commented 2 weeks ago

Could you please add a brief description of flag -skip-unsupported-instructions to the llvm-mca command guide?

peterwaller-arm commented 2 weeks ago

Could you please add a brief description of flag -skip-unsupported-instructions to the llvm-mca command guide?

Done. Text reads:

.. option:: -skip-unsupported-instructions

  Force :program:`llvm-mca` to continue even in the presence of instructions
  which do not parse or lack key scheduling formation. Note that the resulting
  analysis is impacted since those unsupported instructions are ignored as-if
  they are not supplied as a part of the input.
michaelmaitland commented 2 weeks ago

I understand skipping instructions that lack scheduling information but do we really mean to allow instructions that don’t parse? One has to do with conversion from MC to MCA instruction. The other sounds like an inability to generate a MC

peterwaller-arm commented 2 weeks ago

I understand skipping instructions that lack scheduling information but do we really mean to allow instructions that don’t parse?

One example use case of this is in the exynos test modified by this PR. That one has instructions in the test case which are not supported (edit: for some variations of -mcpu tested). Previously, MCA would happily continue to report something anyway. Now that I've changed this not to be default, it's necessary to be able to skip them again to make the test work.

Generally I think this is useful to have. You could find yourself with an assembly you want to analyse which has a rare instruction in it which hasn't yet had LLVM support added, for example if you're using an old version of LLVM at some point in the future and you want to see what MCA would have said if it were not for the (edit: not yet supported at this version) unsupported instructions.

adibiagio commented 2 weeks ago

I understand skipping instructions that lack scheduling information but do we really mean to allow instructions that don’t parse?

One example use case of this is in the exynos test modified by this PR. That one has instructions in the test case which are not supported (edit: for some variations of -mcpu tested). Previously, MCA would happily continue to report something anyway. Now that I've changed this not to be default, it's necessary to be able to skip them again to make the test work.

The fact that previously MCA still generated a report after a parse error is unfortunate.

Ideally, llvm-mca shouldn't skip instructions that don't parse. I prefer if this flag is only used for skipping instructions that don't have scheduling information.

Generally I think this is useful to have. You could find yourself with an assembly you want to analyse which has a rare instruction in it which hasn't yet had LLVM support added, for example if you're using an old version of LLVM at some point in the future and you want to see what MCA would have said if it were not for the (edit: not yet supported at this version) unsupported instructions.

Maybe we should add another flag for this particular use case?

peterwaller-arm commented 2 weeks ago

I'm happy having a separate flag if that seems beneficial (commence bikeshed: -skip-parse-errors? Naming seems tricky given that the parser emits errors if you use instructions which are not supported with the current feature flags, for example).

However, I'm unclear on how beneficial it is to split this case up to a user. A user would still be able to distinguish the two according to the stderr errors (and warnings, notes) that are printed.

Do we really want to force a user to distinguish the reasons for skipping the flags on input? Essentially, if I was faced with a failure due to the presence of some instructions on the input, I would want to reach for the one obvious flag to proceed, rather than to have to figure out which of the N > 1 flags it is.

Another possibility would be to have -skip-unsupported-instructions not be just a bool but a flag that takes multiple options, ranging from -skip-unsupported-instructions=no-sched-info, -skip-unsupported-instructions=parse-failure to -skip-unsupported-instructions=all.

michaelmaitland commented 2 weeks ago

Maybe we should add another flag for this particular use case?

Another possibility would be to have -skip-unsupported-instructions

Either sounds sounds like a good solution. I think there are two distinct cases here:

  1. Cant parse to MCInstr
  2. Cant parse from MCInstr to MCAInstr

We should be careful to not conflate them.

That one has instructions in the test case which are not supported (edit: for some variations of -mcpu tested)

I'm more worried about passing something totally incorrect, like passing not enough registers, or incorrect types of registers, or even mnemonics that don't exist. I also wonder if it would be hygienic to have a test case where you run it on multiple mcpu but one CPU cannot accept that program.

peterwaller-arm commented 2 weeks ago

OK, I propose to:

I just want to clarify what you're asking on this last point; it's that this should test the thing that the exynos case currently hits: we have an instruction where some mcpu can accept the instruction, and others can't, and all have -skip-unsupported-instructions=parse-error set, and it checks that the total cycle count is as expected? I would derive such a test case from the existing exynos case.

michaelmaitland commented 2 weeks ago

To ignore parse errors is to accept programs that are not well formed. Do we really want to support this? Is it useful to have this behavior? What use cases does this have? You wouldn't give a CPU a program with instructions it couldn't support and expect it to work correctly. The compiler wouldn't generate that code for that CPU either.

The proposal doesn't account for this concern:

I'm more worried about passing something totally incorrect, like passing not enough registers, or incorrect types of registers, or even mnemonics that don't exist.

What do you think about this proposal:

peterwaller-arm commented 2 weeks ago

I accept your points. The root of my thinking was to preserve backwards compatibility, but I think there are potential reasons to want this.

To ignore parse errors is to accept programs that are not well formed. Do we really want to support this? Is it useful to have this behavior? What use cases does this have? You wouldn't give a CPU a program with instructions it couldn't support and expect it to work correctly. The compiler wouldn't generate that code for that CPU either.

MCA and LLVM's MC parser seems to already handle bad input quite well, and assembler syntax lends itself is simple enough for this. There aren't lots of complex ambiguities that are introduced by ignoring a line, that I can think of, correct me if I'm wrong.

The analysis outputs may be compromised to some degree for bad inputs, but so long as the user is being explicit about allowing it and being warned that the analysis will be less accurate, I don't see the harm in supporting this case.

the test case should use a different instruction sequence that both CPU can support, or the test case should be split into two cases, one for each CPU.

When I first saw this test case was running with errors behind the scenes I thought this was unfortunate, but later I thought the test case as it stands is pretty neat for what it's worth. It's autogenerated and has all the CPU generations in one file, and the generation lacking support for the instruction neatly omits the unsupported instructions from the output.

I don't feel the case above is very strong, but I'd like to pursue this unless strong objections are raised.

adibiagio commented 2 weeks ago
  • llvm-mca today accepts bad input and prints analysis outputs.
  • It seems likely that was unintentional, but if anyone is relying on this, accidentally or otherwise, it would be nice to 'give them an out' if they need it.

I can tell you that it was not intentional. In case of parse errors, it leads to wrong/misleading perf reports. I may be wrong, but I don't think that this is what most users want. In general, I don't think that people should rely on this.

As I mentioned in my previous message, we could add a separate option to specifically ignore parse errors. When erroring out, we could print a remark message that says "Please use flag -XYZto explicitly ignore parse errors.. That way, people will know how to fix it if they get confused by this behaviour change.

Just my opinion.

michaelmaitland commented 2 weeks ago

I can tell you that it was not intentional. In case of parse errors, it leads to wrong/misleading perf reports. I may be wrong, but I don't think that this is what most users want. In general, I don't think that people should rely on this.

+1

The analysis outputs may be compromised to some degree for bad inputs, but so long as the user is being explicit about allowing it and being warned that the analysis will be less accurate, I don't see the harm in supporting this case.

+1

I am still concerned about the case where the program contains an instruction that is absolute garbage:

like passing not enough registers, or incorrect types of registers, or even mnemonics that don't exist.

I really dislike that we are going to allow this. Can we exclude these kinds of errors from any skip-parse-errors option? Maybe we need two levels: (1) ignore all parse errors and (2) ignore parse errors because CPU did not support those instructions?

Other than that, I would support having an explicit option to skip over parse errors.

michaelmaitland commented 2 weeks ago

I really dislike that we are going to allow this. Can we exclude these kinds of errors from any skip-parse-errors option? Maybe we need two levels: (1) ignore all parse errors and (2) ignore parse errors because CPU did not support those instructions?

On second thought, maybe we just allow the garbage for sake of simplicity. Let users who use this option know that they can abuse it for the sake of backwards compatibility. This approach means this change is minimally invasive to maintainability of llvm-mca since we're keeping it as simple as possible.

You have my support on either approach.

peterwaller-arm commented 2 weeks ago

Thanks for coming around and allowing my side :). I am also motivated to keep things simple.

Ideally the decision isn't permanent; I hope we make a design now which gives a way forward where we can extend things for more specificity if new information comes to light.

So that said I'm swinging back to this proposal, which is my current intent.

https://github.com/llvm/llvm-project/pull/90474#issuecomment-2083198408

OK, I propose to:

  • make llvm-mca report an error and exit by default
  • make -skip-unsupported-instructions take no-sched-info, parse-error and all.
  • parse-error would skip the case Parser->Run(false) returning true (indicating error).

That is:

I'll hold off implementing this for a day or two in case there are more comments refining this.

adibiagio commented 2 weeks ago
  • In aid of keeping things simple we have one flag, -skip-unsupported-instructions, and it takes no-sched-info, parse-error and all. If it becomes desirable to add more distinction it should be possible to add other values to this option.
  • The flag describes what it is doing, which is skipping over instructions on the input.

Sounds reasonable to me. Thanks

holland11 commented 2 weeks ago

Just read through the whole thread and I agree with the consensus up to this point.

  • In aid of keeping things simple we have one flag, -skip-unsupported-instructions, and it takes no-sched-info, parse-error and all. If it becomes desirable to add more distinction it should be possible to add other values to this option.

  • The flag describes what it is doing, which is skipping over instructions on the input.

Implementing it like this seems completely reasonable to me. Thanks for the effort.

peterwaller-arm commented 2 weeks ago

Implemented as proposed, PTAL.

peterwaller-arm commented 1 week ago

Thanks for the input and helping find consensus. I found one more issue which I've addressed: there were some thumb and other target=ARM instructions which weren't parsing. Rather than skip them I've fixed them in situ. I'll merge when I return to work on Tuesday next week assuming that CI is green or there are only trivial things to fix.