This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add Stackmap/Statepoint/Patchpoint support without targets
ClosedPublic

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

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
160

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

166–193

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
139

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.

156

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.

277

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
160

Applied in next patch.

166–193

generateInstSeq is indeed better, thank you!

277

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
277

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.

Zeavee updated this revision to Diff 444721.Jul 14 2022, 10:34 AM

Add case for STACKMAP, STATEPOINT and PATCHPOINT in getInstSizeInBytes.

craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
16

Is this used? I see the discussion about using RISCVMatInt::generateInstSeq but I don't see any code.

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
998 ↗(On Diff #444721)

Use a switch?

Zeavee added inline comments.Jul 14 2022, 10:47 AM
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
16

It will be used in the next patch with targets, but I can remove this import in this one, I missed it.

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
998 ↗(On Diff #444721)

Sure!

Zeavee updated this revision to Diff 444730.Jul 14 2022, 10:50 AM

Use a switch instead of if else for getInstSizeInBytes and remove unused import.

Zeavee updated this revision to Diff 444741.Jul 14 2022, 11:18 AM

Remove Opers temporary variable to avoid compilation error in switch.

Please use update_llc/mir_test_checks.py for all the tests that aren't checking for data directives

Zeavee updated this revision to Diff 445094.Jul 15 2022, 12:09 PM

Update tests that do not check data directive using update_llc_test_checks.py.

jrtc27 added inline comments.Jul 15 2022, 3:17 PM
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
101

Is it still invalid with RVC? You can have 2-byte NOPs there...

llvm/test/CodeGen/RISCV/rv64-patchpoint.ll
4

RISCV doesn't have FastISel support, it is identical to the previous line

llvm/test/CodeGen/RISCV/rv64-stackmap.ll
2

Don't line up despite your attempts to make them

llvm/test/CodeGen/RISCV/stackmap-frame-setup.ll
2

You've got no CHECK lines because this is outputting MIR not assembly, hence you want update_mir_test_checks.py

17–19

Huh?

Zeavee added inline comments.Jul 16 2022, 4:01 AM
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
101

I indeed missed the C_NOP. Should we add one when NumNOPBytes % 4 == 2 then, because emitNops only emits 4 bytes NOPs?

llvm/test/CodeGen/RISCV/rv64-patchpoint.ll
4

I will remove it then.

llvm/test/CodeGen/RISCV/stackmap-frame-setup.ll
17–19

This was added by update_llc_test_checks.py.

Zeavee updated this revision to Diff 445564.EditedJul 18 2022, 10:44 AM

Use 2-bytes NOPs when they are available. Fix tests that used FastISel or an incorrect checks generator.

Why do you have both stackmap.ll and rv64-stackmap.ll? And why does stackmap-frame-setup.ll not have an rv64- prefix like the rest (once that duplication is resolved)?

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
101

I don't know, it depends what you want stackmap code to do, which is something I'm not familiar with. All I know is that, without additional context, requesting 18 bytes of NOPs is a perfectly reasonable thing to do on RISC-V, just as requesting 7 bytes is reasonable on x86 due to it having 1-byte NOPs (0x90). I'll note that @reames already pointed out the discrepancy between emitNops and getNumPatchBytes in terms of bytes vs instructions below.

Also, AsmPrinter::emitNops emits whatever TII->getNop() returns, which is C_NOP on RISC-V if RVC is enabled, so you currently have a bug that this is half the number of bytes being requested in that case.

llvm/test/CodeGen/RISCV/rv64-patchpoint.ll
3

You have a bunch of spaces like this left in various files

llvm/test/CodeGen/RISCV/rv64-stackmap.ll
4

Why's this in a test? This is documentation for how to use llc...

llvm/test/CodeGen/RISCV/stackmap.ll
4

Ditto

Zeavee updated this revision to Diff 445586.Jul 18 2022, 11:53 AM

Fixed spaces issues in tests, added correct prefixes and merged duplicates.

Why do you have both stackmap.ll and rv64-stackmap.ll? And why does stackmap-frame-setup.ll not have an rv64- prefix like the rest (once that duplication is resolved)?

I mirrored the AArch64 stackmap tests to try to include all tests cases. But I will add rv64- prefixes if it is better.

Zeavee updated this revision to Diff 524414.May 22 2023, 11:51 AM

Rebased the patch on main. For context, this patch has been used by the LLVM backend of GraalVM's Native Image project in production for around 4 months with no major issues.

evandro removed a subscriber: evandro.May 22 2023, 5:24 PM
reames accepted this revision.May 25 2023, 3:24 PM

LGTM

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
241

You can add the new cases here to the switch above.

This revision is now accepted and ready to land.May 25 2023, 3:24 PM
Zeavee updated this revision to Diff 526017.May 26 2023, 3:59 AM

Remove duplicate switch in RISCVAsmPrinter::emitInstruction

Zeavee updated this revision to Diff 526023.May 26 2023, 4:13 AM

Rebase patch on main

Zeavee added a comment.Aug 2 2023, 6:23 AM

Hello, sorry for the ping, but the revision has been accepted and the requested changes applied. Would it be possible to have it merged in the main branch?

Zeavee updated this revision to Diff 557675.Oct 10 2023, 9:52 AM

Rebased patch on main branch.

This revision was automatically updated to reflect the committed changes.