-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[llvm-exegesis] Print generated assembly snippet #142540
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-exegesis] Print generated assembly snippet #142540
Conversation
|
@llvm/pr-subscribers-tools-llvm-exegesis Author: Lakshay Kumar (lakshayk-nv) ChangesDebug generated disassembly by passing argument Full diff: https://github.com/llvm/llvm-project/pull/142540.diff 1 Files Affected:
diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
index a7771b99e97b1..7c2a6c966106a 100644
--- a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
+++ b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
@@ -20,6 +20,7 @@
#include "llvm/ADT/Twine.h"
#include "llvm/Config/llvm-config.h" // for LLVM_ON_UNIX
#include "llvm/Support/CrashRecoveryContext.h"
+#include "llvm/Support/Debug.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
@@ -29,6 +30,7 @@
#include <cmath>
#include <memory>
#include <string>
+#define DEBUG_TYPE "exegesis-benchmark-runner"
#ifdef __linux__
#ifdef HAVE_LIBPFM
@@ -709,6 +711,38 @@ std::pair<Error, Benchmark> BenchmarkRunner::runConfiguration(
}
outs() << "Check generated assembly with: /usr/bin/objdump -d "
<< *ObjectFilePath << "\n";
+
+#ifdef __linux__
+ int StdOutFD, StdErrFD;
+ SmallString<128> StdOutFile, StdErrFile;
+ sys::fs::createTemporaryFile("temp-objdump-out", "txt", StdOutFD,
+ StdOutFile);
+ sys::fs::createTemporaryFile("temp-objdump-err", "txt", StdErrFD,
+ StdErrFile);
+ std::vector<std::optional<StringRef>> Redirects = {
+ std::nullopt, // stdin
+ StringRef(StdOutFile), // stdout
+ StringRef(StdErrFile) // stderr
+ };
+
+ std::string ErrMsg;
+ int Result = sys::ExecuteAndWait(
+ "/usr/bin/objdump", {"/usr/bin/objdump", "-d", *ObjectFilePath},
+ std::nullopt, Redirects, 0, 0, &ErrMsg);
+ auto StdOutBuf = MemoryBuffer::getFile(StdOutFile);
+ if (StdOutBuf && !(*StdOutBuf)->getBuffer().empty())
+ LLVM_DEBUG(dbgs() << "[llvm-exegesis][objdump] Generated assembly:\n"
+ << (*StdOutBuf)->getBuffer() << '\n');
+ auto StdErrBuf = MemoryBuffer::getFile(StdErrFile);
+ if (StdErrBuf && !(*StdErrBuf)->getBuffer().empty())
+ LLVM_DEBUG(dbgs() << "[llvm-exegesis][objdump] stderr:\n"
+ << (*StdErrBuf)->getBuffer() << '\n');
+ if (!ErrMsg.empty())
+ LLVM_DEBUG(dbgs() << "[llvm-exegesis][objdump] process error: " << ErrMsg
+ << '\n');
+ sys::fs::remove(StdOutFile);
+ sys::fs::remove(StdErrFile);
+#endif
}
if (BenchmarkPhaseSelector < BenchmarkPhaseSelectorE::Measure) {
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't exactly see the point of this. Running objdump (or llvm-objdump/whatever version) on the output of -dump-object-to-disk is intended to cover this exact use case. I don't think the slight convenience of running it automatically by passing -debug-only or -debug outweights the extra code complexity and other problems that will come with this.
It's hard to test because now you're relying on /usr/bin/objdump existing, you have no idea what /usr/bin/objdump actually does or if it even exists, and creating temporary files to invoke an external command is the wrong way to go about this. If we want to print instructions we can spin up a MCInstPrinter to print the vector of MCInsts that we have earlier. That case is still covered by -dump-object-to-disk though.
This would be a very welcome debug feature for me. I agree that this can be achieved in the way you described it, but it would be so much more efficient for me if I can flip a switch and don't need to do this all manually. Analysing the snippets is really important, i.e. analysing them to see if they truly test what they want to test is important and time consuming, and anything that helps is welcome. I agree with the trade-off in general, a debug feature shouldn't lead to massive churn in the code, or pose maintainance problems now or later, but this really doesn't seem to be the case here. I.e., it is self-contained, and not a lot of code, partly because it already relies on the infrastructure that is mostly there already. Long story short, I fully support the change, but I would like to see 2 changes:
|
|
I had actually not looked at the implementation very carefully, but now looking and thinking about it, I think I would like to have this available under and option, so that I can toggle this on or off. For example, add an option In terms of implementation, this could be a wrapper around "-dump-object-to-disk" and then disassembling and printing it to stdout, or the current implementation. Does this (and my previous comment) help with your concerns, @boomanaiden154 ? |
The snippet (not repeated) is already shown in the benchmark key: I don't see how this patch will provide significant benefit over that, other than maybe printing everything in a more familiar format.
I don't see how this is preferrable to a |
…xternal dependency outside llvm project
…vm-project into llvm-exegesis-disassembly
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I was just looking at this, and noticed the update, and was wondering why all the pointer authentication stuff is included here. I am assuming that was included by mistake? |
Apologies, I messed up this branch. I just wanted to add the testcase commit. |
a8160ae to
b57c3ab
Compare
| }; | ||
|
|
||
| // Preview generated assembly snippet | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work now? Does this mean the snippet is printed twice if option preview-gen-assembly is provided and it is a debug build, is that right?
And --debug-only="preview-gen-assembly" only works with a debug build?
I would prefer to just flip a switch and option, similar to --dump-object-to-disk, so that this works in any build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to use:
--debug-only="print-gen-assembly" debugs the whole generated assembly snippet .
--debug-only=preview-gen-assembly debugs the initial 10 instructions and ending 3 lines.
(Updated PR Description accordingly)
No, Given argument of --debug-only="print-gen-assembly" exegesis will print out the generated assembly only once with truncated middle part, so not to fill up whole terminal for large value of min-instructions
$ build/bin/llvm-exegesis -mcpu=neoverse-v2 --dump-object-to-disk=%d --benchmark-phase=measure --min-instructions=10000 --mode=latency --opcode-name=ADCXr --debug-only="preview-gen-assembly"
setRegTo is not implemented, results will be unreliable
setRegTo is not implemented, results will be unreliable
Generated assembly snippet:
'''
0: f81f0ff7 str x23, [sp, #-16]!
4: d2800017 mov x23, #0
8: d280000d mov x13, #0
c: 9a0d02ed adc x13, x23, x13
10: 9a0d02ed adc x13, x23, x13
14: 9a0d02ed adc x13, x23, x13
18: 9a0d02ed adc x13, x23, x13
1c: 9a0d02ed adc x13, x23, x13
20: 9a0d02ed adc x13, x23, x13
24: 9a0d02ed adc x13, x23, x13
... (9992 more instructions)
9c48: 9a0d02ed adc x13, x23, x13
9c4c: f84107f7 ldr x23, [sp], #16
9c50: d65f03c0 ret
'''
Check generated assembly with: /usr/bin/objdump -d %d
---
mode: latency
key:
instructions:
- 'ADCXr X13 X23 X13'
config: ''
register_initial_values:
- 'X23=0x0'
- 'X13=0x0'
- 'NZCV=0x0'
cpu_name: neoverse-v2
llvm_triple: aarch64-unknown-linux-gnu
min_instructions: 10000
measurements:
- { key: latency, value: 1.0036, per_snippet_value: 1.0036, validation_counters: {} }
error: ''
info: Repeating a single explicitly serial instruction
assembled_snippet: F70F1FF8170080D20D0080D2ED020D9AED020D9AED020D9AED020D9AF70741F8C0035FD6
...
If we explicitly give argument --debug-only="preview-gen-assembly,print-gen-assembly" to exegesis, both preview and print debug will result in duplicate print.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And --debug-only="preview-gen-assembly" only works with a debug build?
No, I checked this work for both Debug and Release build (UPDATE:with assertion on).
I would prefer to just flip a switch and option, similar to --dump-object-to-disk, so that this works in any build.
Given that, Can you reconsider this (additional argument) because we will require two arguments if not for --debug-only one for whole assembly print and another for preview.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Currently, Given the argument of --dump-object-to-disk=<filename> exegesis just stdout
Check generated assembly with: /usr/bin/objdump -d %d. This object file is to be objdump in next command outside exegesis)
In terms of implementation, this could be a wrapper around "-dump-object-to-disk"
I know you previously also asked for this. If this seems necessary, can you please expand on wrapper thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarifications.
What I have suggested could have been an alternative way to implement this, but I am happy with the implementation here as it is nice and concise and doesn't depend on external tools. So, ignore that suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, this looks a lot better.
| } | ||
| if (N > (PreviewFirst + PreviewLast)) { | ||
| if (Preview) { | ||
| dbgs() << "...\t(" << (N - PreviewFirst - PreviewLast) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yeah, debug vs. release builds had me confused earlier. I guess this debug stream is available even in non-Debug mode, but does it make sense to use outs() instead? I see that being used elsewhere, and guess that is the slightly more appropriate output stream for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the output and think this will be very useful to me. Ideally I would like to have this available in a release build without assertions, but let's get some experience first before we promote this to an option, so LGTM.
I've left two nits that don't need another review if addressed.
Why the preference for enabling this without assertions? This seems almost exclusively a debug feature. |
|
Sorry for the confusion and putting noise on the line, but @boomanaiden154 was happy with this as a debug feature. Let's go with that. Hopefully it's not too much work to restore the LLVM_DEBUG and dbgs() things. |
|
Introduced back this as debug feature. Thanks ! |
A last nit: it looks like we are only testing |
Added the same, Thanks! |
| @@ -0,0 +1,21 @@ | |||
| REQUIRES: aarch64-registered-target | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs to require an assertions enabled build since you're using debug macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added asserts in testfile. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor change that is needed, otherwise LGTM.
| @@ -0,0 +1,21 @@ | |||
| REQUIRES: aarch64-registered-target, asserts | |||
|
|
|||
| RUN: llvm-exegesis -mcpu=neoverse-v2 --benchmark-phase=measure --min-instructions=100 --mode=latency --debug-only=preview-gen-assembly --opcode-name=ADDVv4i16v 2>&1 | FileCheck %s -check-prefix=PREVIEW | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you have to progress to --benchmark-phase=measure, you also need a requires for being able to use perf counters. You might be able to get away with --use-dummy-perf-counters though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
@sjoerdmeijer I've landed 0f77887 to fix a warning from this PR. Thanks! |
Oopsy, thanks a lot @kazutakahirata |
Debug generated disassembly by passing argument
debug-only="print-gen-assembly"ordebug-only=preview-gen-assemblyof exegesis call.--debug-only="print-gen-assembly"debugs the whole generated assembly snippet .--debug-only=preview-gen-assemblydebugs the initial 10 instructions and ending 3 lines.Thus, We can in glance see the initial setup code like registers setup and instruction followed by truncated middle and finally print out the last 3 instructions.
This helps us look into assembly that exegesis is execution in hardware, Thus, it is simply functionally alias to separate objdump command on the dumped object file.
Just a good to have change for dev experience. :)
Please review: @sjoerdmeijer, @boomanaiden154, @davemgreen