Sorry for the delay in getting back to you. I've updated the NOP sled generation and tested it out, it now seems to work with compressed instructions - the patched sleds were identical to what I'd previously observed when compiling with march=rv64g. When I had originally written this in 2021/early 2022, some of the instructions were getting corrupted when they were being patched in - either this was a bug in my implementation that has been fixed over the course of the review process, or it might've been an issue with qemu or llvm at the time that has been fixed in the years since.

Sorry - I missed adding updates with the last diff - I think I've addressed most of the comments. Just realized that the diff didn't include context, updated that as well.

Ok, I've tried to make the changes you've suggested @MaskRay , let me know if I've missed something.

In D117929#4445858, @MaskRay wrote:

I am still interested in a RISC-V XRay port :)

In D117929#3314355, @ashwin98 wrote:

In D117929#3299751, @dberris wrote:

In D117929#3276843, @ashwin98 wrote:

Fixed another lint issue, they should all be done for now hopefully.

@dberris Sorry, I'm a little confused, do you mean I need to update some of the clang tests for XRay instrumentation to test compilation and linking, or add new ones for the llvm-xray tool under llvm/test/tools?

Yes to both. :)

There are already some tests that ensure the supported triples build with XRay instrumentation and that we can get the instrumentation map with the tooling on binaries generated with clang+lld. Extending those to support risc64 and ensure we're able to find the instrumentation with the tooling gives us higher confidence that this will work when the patch is accepted.

Before running the tests, I've been testing some of the subcommands with the generated logs. When xray account or stack was run directly on the log file, the data for the instrumented functions was output (I believe the XRay tool recognizes it as a YAML file and processes it as such). However, when passing the --instr_map flag, issues crop up. I originally ran into an error about the executable not being an ELF binary. I confirmed that clang was generating ELF binaries. There was a test checking the triple architecture in InstrumentationMap.cpp that seemed to be causing this.

On including RISCV64 to the check, I see new issues while reading the instrumentation map. How do I verify that the instrumentation map is being generated correctly (or if there is a problem with it), and where is the code that is responsible for the generation of the instrumentation map (is it in xray_init.cpp)? I'm not sure if this is a RISCV compatibility issue with the xray tool, or if I've missed something that is causing problems during the instrumentation map initialization.

In D117929#3299751, @dberris wrote:

In D117929#3276843, @ashwin98 wrote:

Fixed another lint issue, they should all be done for now hopefully.

@dberris Sorry, I'm a little confused, do you mean I need to update some of the clang tests for XRay instrumentation to test compilation and linking, or add new ones for the llvm-xray tool under llvm/test/tools?

Yes to both. :)

There are already some tests that ensure the supported triples build with XRay instrumentation and that we can get the instrumentation map with the tooling on binaries generated with clang+lld. Extending those to support risc64 and ensure we're able to find the instrumentation with the tooling gives us higher confidence that this will work when the patch is accepted.

Removed extra triples from the test.

Updated the riscv64 sled function to fix the addition/shift operations and get rid of the superfluous slli and srli instructions. Cut out unnecessary comments in the ASM Printer.

Fixed another lint issue, they should all be done for now hopefully.

Made changes to handle lint issues.

Updated the diff, made the following changes:

1. Merged the riscv files into xray_riscv.cpp and removed the if-else code for %hi()
2. Cleaned up the issues related to indenting and comments in RISCVAsmPrinter.cpp
3. Updated the test file to pass -verify-machineinstrs and remove unnecessary attributes as well as {{.*}}s
4. Fixed riscv32 comments - it is now only commented out in cmake/Modules/AllSupportedArchDefs.cmake

Thank you for your feedback! I could combine the riscv32 and 64 cpp files with some xlen conditions if that will work better, but that might take a bit of a hit in terms of readability (do I explain both sleds in the comments preceding the implementation). I have commented riscv32 out because I haven't managed to test it out yet, I've had some difficulty getting llvm set up for riscv 32 - I will leave riscv32 commented out only in the cmake file.