Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
May 7, 2024

Conversation

peterwaller-arm
Copy link
Contributor

@peterwaller-arm peterwaller-arm commented Apr 29, 2024

[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

Copy link

github-actions bot commented Apr 29, 2024

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

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 29, 2024

@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 --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<const CodeRegions &> AsmCodeRegionGenerator::parseCodeRegions(
-    const std::unique_ptr<MCInstPrinter> &IP) {
+    const std::unique_ptr<MCInstPrinter> &IP,
+    bool SkipUnsupportedInstructions) {
   MCTargetOptions Opts;
   Opts.PreserveAsmComments = false;
   CodeRegions &Regions = getRegions();
@@ -61,7 +62,9 @@ Expected<const CodeRegions &> AsmCodeRegionGenerator::parseCodeRegions(
         "This target does not support assembly parsing.",
         inconvertibleErrorCode());
   Parser->setTargetParser(*TAP);
-  Parser->Run(false);
+  if (Parser->Run(false) && !SkipUnsupportedInstructions)
+    return make_error<StringError>("Assembly input parsing had errors.",
+                                   inconvertibleErrorCode());
 
   if (CCP->hadErr())
     return make_error<StringError>("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<const CodeRegions &>
-  parseCodeRegions(const std::unique_ptr<MCInstPrinter> &IP) = 0;
+  parseCodeRegions(const std::unique_ptr<MCInstPrinter> &IP,
+                   bool SkipUnsupportedInstructions) = 0;
 
 public:
   CodeRegionGenerator() {}
@@ -164,7 +165,8 @@ class AnalysisRegionGenerator : public virtual CodeRegionGenerator {
   AnalysisRegionGenerator(llvm::SourceMgr &SM) : Regions(SM) {}
 
   virtual Expected<const AnalysisRegions &>
-  parseAnalysisRegions(const std::unique_ptr<MCInstPrinter> &IP) = 0;
+  parseAnalysisRegions(const std::unique_ptr<MCInstPrinter> &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<const InstrumentRegions &>
-  parseInstrumentRegions(const std::unique_ptr<MCInstPrinter> &IP) = 0;
+  parseInstrumentRegions(const std::unique_ptr<MCInstPrinter> &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<const CodeRegions &>
-  parseCodeRegions(const std::unique_ptr<MCInstPrinter> &IP) override;
+  parseCodeRegions(const std::unique_ptr<MCInstPrinter> &IP,
+                   bool SkipUnsupportedInstructions) override;
 };
 
 class AsmAnalysisRegionGenerator final : public AnalysisRegionGenerator,
@@ -222,8 +226,10 @@ class AsmAnalysisRegionGenerator final : public AnalysisRegionGenerator,
   MCStreamerWrapper *getMCStreamer() override { return &Streamer; }
 
   Expected<const AnalysisRegions &>
-  parseAnalysisRegions(const std::unique_ptr<MCInstPrinter> &IP) override {
-    Expected<const CodeRegions &> RegionsOrErr = parseCodeRegions(IP);
+  parseAnalysisRegions(const std::unique_ptr<MCInstPrinter> &IP,
+                       bool SkipUnsupportedInstructions) override {
+    Expected<const CodeRegions &> RegionsOrErr =
+        parseCodeRegions(IP, SkipUnsupportedInstructions);
     if (!RegionsOrErr)
       return RegionsOrErr.takeError();
     else
@@ -231,8 +237,10 @@ class AsmAnalysisRegionGenerator final : public AnalysisRegionGenerator,
   }
 
   Expected<const CodeRegions &>
-  parseCodeRegions(const std::unique_ptr<MCInstPrinter> &IP) override {
-    return AsmCodeRegionGenerator::parseCodeRegions(IP);
+  parseCodeRegions(const std::unique_ptr<MCInstPrinter> &IP,
+                   bool SkipUnsupportedInstructions) override {
+    return AsmCodeRegionGenerator::parseCodeRegions(
+        IP, SkipUnsupportedInstructions);
   }
 };
 
@@ -254,8 +262,10 @@ class AsmInstrumentRegionGenerator final : public InstrumentRegionGenerator,
   MCStreamerWrapper *getMCStreamer() override { return &Streamer; }
 
   Expected<const InstrumentRegions &>
-  parseInstrumentRegions(const std::unique_ptr<MCInstPrinter> &IP) override {
-    Expected<const CodeRegions &> RegionsOrErr = parseCodeRegions(IP);
+  parseInstrumentRegions(const std::unique_ptr<MCInstPrinter> &IP,
+                         bool SkipUnsupportedInstructions) override {
+    Expected<const CodeRegions &> RegionsOrErr =
+        parseCodeRegions(IP, SkipUnsupportedInstructions);
     if (!RegionsOrErr)
       return RegionsOrErr.takeError();
     else
@@ -263,8 +273,10 @@ class AsmInstrumentRegionGenerator final : public InstrumentRegionGenerator,
   }
 
   Expected<const CodeRegions &>
-  parseCodeRegions(const std::unique_ptr<MCInstPrinter> &IP) override {
-    return AsmCodeRegionGenerator::parseCodeRegions(IP);
+  parseCodeRegions(const std::unique_ptr<MCInstPrinter> &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<const mca::AnalysisRegions &> 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<const mca::InstrumentRegions &> InstrumentRegionsOrErr =
-      IRG.parseInstrumentRegions(std::move(IPtemp));
+      IRG.parseInstrumentRegions(std::move(IPtemp),
+                                 SkipUnsupportedInstructions);
   if (!InstrumentRegionsOrErr) {
     if (auto Err = handleErrors(InstrumentRegionsOrErr.takeError(),
                                 [](const StringError &E) {

@adibiagio adibiagio self-requested a review April 29, 2024 14:30
@adibiagio
Copy link
Collaborator

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

@peterwaller-arm
Copy link
Contributor Author

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
Copy link
Contributor

michaelmaitland commented Apr 29, 2024

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
Copy link
Contributor Author

peterwaller-arm commented Apr 29, 2024

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
Copy link
Collaborator

adibiagio commented Apr 29, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

peterwaller-arm commented Apr 29, 2024

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).
  • Add a test case to cover your suggestion '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.'

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
Copy link
Contributor

michaelmaitland commented Apr 29, 2024

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:

  • 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.
  • -skip-unsupported-instructions only skips instructions that cannot be converted from MCInst to mca::Instruction

@peterwaller-arm
Copy link
Contributor Author

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.

  • 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.
  • tomorrow's new instructions are today's malformed inputs. Yes, one hopes LLVM will gain support for those new instructions, and that a user will be able to upgrade their llvm-mca to gain support of those, but I still think it is useful to have an alternative route if either of these conditions are false for some time.
  • Imagine a case where someone is trying to bisect a behaviour change in llvm-mca, or study mca's output as a function of time for a given (large) set of inputs. They could find themselves running an old llvm-mca against new code. If that use case involved throwing 'lots of code at llvm-mca', it may be useful to ignore instructions which don't parse at older revisions of llvm.

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
Copy link
Collaborator

  • 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 -XYZ to 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

#90474 (comment)

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:

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

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

@adibiagio
Copy link
Collaborator

adibiagio commented May 1, 2024

  • 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
Copy link
Contributor

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
Copy link
Contributor Author

Implemented as proposed, PTAL.

  • Also added a table to the PR body which shows the test coverage.
  • Also added a release note and updated the command guide.

Copy link
Collaborator

@adibiagio adibiagio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thanks!

Copy link
Contributor

@michaelmaitland michaelmaitland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for addressing this.

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
any           | * | (Logically covered by the other cases)
@peterwaller-arm
Copy link
Contributor Author

peterwaller-arm commented May 2, 2024

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.

@peterwaller-arm peterwaller-arm merged commit 1de0535 into llvm:main May 7, 2024
4 of 5 checks passed
@peterwaller-arm peterwaller-arm deleted the abort-on-parse-error branch May 7, 2024 08:13
# RUN: not llvm-mca -skip-unsupported-instructions=none %s -o /dev/null 2>&1 | FileCheck --check-prefixes=CHECK-ALL,CHECK %s
# RUN: not llvm-mca -skip-unsupported-instructions=lack-sched %s -o /dev/null 2>&1 | FileCheck --check-prefixes=CHECK-ALL,CHECK %s
# RUN: not llvm-mca -skip-unsupported-instructions=parse-failure %s -o /dev/null 2>&1 | FileCheck --check-prefixes=CHECK-ALL,CHECK-SKIP %s
# RUN: not llvm-mca -skip-unsupported-instructions=any %s -o /dev/null 2>&1 | FileCheck --check-prefixes=CHECK-ALL,CHECK-SKIP %s
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference: https://lab.llvm.org/buildbot/#/builders/60/builds/17086

I've just become aware that this appears to be failing on builders with messages like:

not c:\buildbot\as-builder-1\x-armv7l\build\bin\llvm-mca.exe C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\test\tools\llvm-mca\bad-input.s -o /dev/null 2>&1 | c:\buildbot\as-builder-1\x-armv7l\build\bin\filecheck.exe --check-prefixes=CHECK-ALL,CHECK C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\test\tools\llvm-mca\bad-input.s
# executed command: not 'c:\buildbot\as-builder-1\x-armv7l\build\bin\llvm-mca.exe' 'C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\test\tools\llvm-mca\bad-input.s' -o /dev/null
# executed command: 'c:\buildbot\as-builder-1\x-armv7l\build\bin\filecheck.exe' --check-prefixes=CHECK-ALL,CHECK 'C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\test\tools\llvm-mca\bad-input.s'
# .---command stderr------------
# | C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\test\tools\llvm-mca\bad-input.s:11:10: error: CHECK: expected string not found in input
# | # CHECK: error: Assembly input parsing had errors, use -skip-unsupported-instructions=parse-failure to drop failing lines from the input.
# |          ^
# | <stdin>:1:1: note: scanning from here
# | 'skylake-avx512' is not a recognized processor for this target (ignoring processor)
# | ^
# | 
# | Input file: <stdin>
# | Check file: C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\test\tools\llvm-mca\bad-input.s
# | 
# | -dump-input=help explains the following input dump.
# | 
# | Input was:
# | <<<<<<
# |           1: 'skylake-avx512' is not a recognized processor for this target (ignoring processor) 
# | check:11     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
# |           2: 'skylake-avx512' is not a recognized processor for this target (ignoring processor) 
# | check:11     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | >>>>>>
# `-----------------------------
# error: command failed with exit status: 1

The bots for which this have failed appear to be cross-compilers which don't have the native build target enabled.

It seems the safest immediate fix is to move/copy this test to x86/arm so that there is some coverage for this test for two common targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposed fix committed as afc10fc for now. I'll monitor the bots.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite there. Looks like we can't rely on the default mcpu for bad inputs, since this fails if we run on a CPU for which there is no scheduling information.

So I've gone for supplying mtriple and mcpu for this case, committed in 458d706.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patches LGTM.

peterwaller-arm added a commit that referenced this pull request May 7, 2024
... for now.

This is a follow up to #90474 in response to build bot failures.

This test is intended to check a case where invalid assembly is passed
to llvm-mca.

Unfortunately it appears that a cross-toolchain built with
-DTOOLCHAIN_TARGET_TRIPLE does not have an llvm-mca which works out of
the box if the host target is not enabled.

As a quick fix to make the build bots green, move the test into AArch64
and X86 so that there is reasonable coverage for this test; later I hope
mca can be fixed to work out of the box in this configuration.
peterwaller-arm added a commit that referenced this pull request May 7, 2024
Note: This patch is distinct from the previous one titled
  "[llvm-mca] Move bad-input.s test to be target specific"

This is a followup to #90474 and commit
afc10fc

Context: Builders failing because they're unable to run the failure
test.

This still doesn't work in various circumstances, it seems MCA doesn't
want to run on a wide variety of hosts in various configurations, so
stick to the tried and tested method and pass -mtriple and -mcpu.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants