Page MenuHomePhabricator

[RISCV] Add Stackmap/Statepoint/Patchpoint support without targets
Needs ReviewPublic

Authored by Zeavee on Apr 11 2022, 4:36 AM.

Details

Summary

This patch adds stackmap support for RISC-V without targets (i.e. the nop patchable forms).

Diff Detail

Unit TestsFailed

TimeTest
60,100 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/scariness_score_test.cpp.tmp
60,050 msx64 debian > libFuzzer.libFuzzer::large.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LargeTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/large.test.tmp-LargeTest

Event Timeline

Zeavee created this revision.Apr 11 2022, 4:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 4:36 AM
Zeavee requested review of this revision.Apr 11 2022, 4:36 AM
Zeavee updated this revision to Diff 421898.Apr 11 2022, 6:19 AM
This comment was removed by Zeavee.
Zeavee updated this revision to Diff 421900.Apr 11 2022, 6:22 AM

Removes RISCVMCInstLower.h which was not used.

Zeavee updated this revision to Diff 421920.Apr 11 2022, 7:26 AM

Adds a missing 0 IMM in a JALR instruction.

asb added a comment.Apr 11 2022, 7:56 AM

Thanks for the contribution! I'm not very familiar with the stackmap/patchpoint/statepoint infrastructure so it may take a bit of time to review this (if anyone else on the CC list has looked at it before, please do dive in!).

One quick question: do you expect this to work on RV32 as well? If so, it would be good to have test coverage for that also.

Hello! I am happy to help! Since it is my first contribution, I am not yet familiar with everything, so it might be a little bit messy at the start, sorry in advance.

For your question, I only implemented it for RV64 for another project, and from what I can think of now, the only issue for the current implementation to work on RV32 would be the method RISCVAsmPrinter::LowerPATCHPOINT, as it materializes a 64 bits jump address. From what I see in the other architectures, the materialized jump address always seems to be 64 bits (if I did not miss anything). Thus, either it can work for 32 bits architecture too, or those architectures only implement 64 bits. I will try locally to check this.

Zeavee updated this revision to Diff 421938.Apr 11 2022, 8:44 AM

Removes unnecessary space on empty line.

I looked a bit deeper into a RV32 implementation, but in fact, according to this https://llvm.org/docs/StackMaps.html#stack-map-format, stackmap support is currently only implemented for 64-bit platforms. Thus it might require more work in the architecture independent part of stackmap, which I am not totally familiar with for now and I also imagine it would be better to start with the more common architectures instead of RV32.

Don't forget to include the full context in your patches. See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
It looks like you also want to use clang-format to reformat the code.

Shouldn't the lowering code check that the subtarget is RV64, and not RV32?

Zeavee updated this revision to Diff 422234.Apr 12 2022, 7:56 AM

Sorry for the delay, it took me some time to run clang-format. I also added the context as asked. Finally, I added an assert to make sure STACKMAP, PATCHPOINT and STATEPOINT are only used with RV64.

asb added a comment.Apr 13 2022, 8:33 AM

Sorry for the delay, it took me some time to run clang-format. I also added the context as asked. Finally, I added an assert to make sure STACKMAP, PATCHPOINT and STATEPOINT are only used with RV64.

I'm not seeing the codepaths for the case where the intrinsics are used on RV32? You'll want to use report_fatal_error rather than assert (as the assert wont' be checked on release builds).

Zeavee updated this revision to Diff 422529.Apr 13 2022, 8:48 AM

I replaced assert by report_fatal_error to indeed stop the execution when reaching this point. I did not include any code for when the intrinsics are run on RV32 as accordingly to the LLVM documentation, Stack map is a 64 bits feature. Thus I only implemented it for RV64. Should I do it for RV32 too?

asb added inline comments.Apr 13 2022, 8:58 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9529

I think you mean !Subtarget.is64Bit()?

9531

Nit: maybe "are only supported on 64-bit targets"?

Zeavee added inline comments.Apr 13 2022, 9:01 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
9529

Indeed, sorry.

Zeavee updated this revision to Diff 422532.Apr 13 2022, 9:07 AM

Added requested changes on error when code is executed on RV32.

luismarques added inline comments.Apr 30 2022, 7:24 AM
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
159

Nit: put some ' digit separators to make it easier to read the constant. E.g. 0x0xFFFF'FFFF'FFFF. (Or use some higher-level function from MathExtras.h if there is one).

165–192

Can't we do better than this for materializing the address? If the number of instructions doesn't have to be fixed you can even use generateInstSeq from RISCVMatInt.h to generate a more optimal instruction sequence.

reames requested changes to this revision.May 1 2022, 8:44 AM
reames added a subscriber: reames.
reames added inline comments.
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
138

You repeat this code a number of times. Please pull out a helper function EmitNops(N), and maybe one for EmitNop as well.

You might be able to reuse AP.emitNops here, but that returns a fixed number of nops, not a fixed number of nop bytes. Looking at the x86 parallel, I think that's what you want here.

155

CallTarget can be a global symbol as well as a constant.

I'd suggest splitting this patch into support for patchpoint/statapoint without targets (i.e. the nop patchable forms), and then adding the target resolution separately. The call resolution adds a decent amount of complexity, and is worth reviewing in detail on it's own.

172

Seeing this on the AsmPrinter is suprising. I'd have expected this code to be in RISCVMCInstLower.cpp. Is there something I'm missing here?

This revision now requires changes to proceed.May 1 2022, 8:44 AM
Zeavee updated this revision to Diff 426961.May 4 2022, 4:26 AM
Zeavee retitled this revision from Add Stackmap support for RISC-V to [RISCV] Add Stackmap/Statepoint/Patchpoint support without targets.
Zeavee edited the summary of this revision. (Show Details)

I removed the code related to the target resolution, to put it in another patch as suggested. I still took the comments into account and they will be applied in the other patch. I also used emitNops to avoid code duplication when emitting the Nops.

Zeavee marked 6 inline comments as done.May 4 2022, 4:33 AM
Zeavee added inline comments.
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
159

Applied in next patch.

165–192

generateInstSeq is indeed better, thank you!

172

It seems to be the case for most architectures. It is true however that the lowering of those instructions should be in RISCVMCInstLower.cpp as it would be more consistent. Should I move it there?

reames added inline comments.May 4 2022, 2:32 PM
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
172

You should ignore me here. On a quick skim, I was confusing being in the MCInstLowering with not being on the AsmPrinter class. I was wrong, and you have the right placement.