This is an archive of the discontinued LLVM Phabricator instance.

[IR] Add GraalVM calling conventions
ClosedPublic

Authored by Zeavee on May 22 2023, 7:59 AM.

Details

Summary

Adds GraalVM calling conventions. The only difference with the default calling conventions is that GraalVM reserves two registers for the heap base and the thread. Since the registers are then accessed by name, getRegisterByName has to be updated accordingly.

This patch implements the calling conventions only for X86, AArch64 and RISC-V.

For X86, the reserved registers are X14 and X15. For AArch64, they are X27 and X28. For RISC-V, they are X23 and X27.

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.

Diff Detail

Event Timeline

Zeavee created this revision.May 22 2023, 7:59 AM
Zeavee requested review of this revision.May 22 2023, 7:59 AM
Zeavee added a comment.Aug 2 2023, 6:28 AM

Hello, sorry for the ping. Would it be possible to have a review for this patch? I am sadly not sure if I should assign a specific reviewer for this change, so if that is the case, please tell me.

I'm not very familiar with the CallingConv code but the X86 changes look fine to me.

It would be nice to add some test coverage of course.

Zeavee updated this revision to Diff 553952.Aug 28 2023, 8:29 AM

Add tests for GraalVM calling conventions

asb added a comment.EditedSep 5 2023, 5:35 AM

Almost all the other added calling conventions seem to additionally add a new token so it can be named rather than using cc $someint. e.g. ghccc. Wouldn't it make sense to do the same here?

Just a few minor comments for RISC-V:

  • We typically cover both rv64 and rv32 in tests. Your CHECK-NOT lines seem to be written with this in mind, but you only have an RV64 RUN line. The filename can be updated of course - grallcc.ll should be fine I think.
  • We normally use the minimum -mtriple needed to provoke the desired test output. In your case I think just -mtriple=riscv64 would be sufficient
  • We don't have RVE codegen support right now, but it would probably be sensible to report_fatal_error if Subtarget.isRVE (which has only 16 GPRs) as the calling convention clearly wouldn't be cmpatible there.

Thanks!

Zeavee updated this revision to Diff 556020.EditedSep 6 2023, 6:23 AM

Add graalcc token for Graal calling convention, rename test files to graalcc.ll, report_fatal_error if Subtarget.isRVE(), test for riscv32 and use minimal -mtriple.

Zeavee added a comment.Oct 9 2023, 3:48 AM

Hello, gentle ping for a review.

Zeavee updated this revision to Diff 557775.Oct 19 2023, 7:33 AM

Rebase patch and ping.

Zeavee added a comment.Nov 8 2023, 1:16 AM

Friendly ping

RKSimon accepted this revision.Nov 16 2023, 7:16 AM

LGTM - @asb any more comments?

This revision is now accepted and ready to land.Nov 16 2023, 7:16 AM
asb accepted this revision.Nov 16 2023, 7:41 AM

LGTM. For the changes in lvm/lib/IR/AsmWriter.cpp we'd typically match the style of the surrounding code even though clang-format disagrees.

Zeavee updated this revision to Diff 558125.Nov 17 2023, 6:49 AM

Rebase patch and use same format in switches to keep same style.

Thank you! I sadly do not have commit rights, so please could someone commit this patch on my behalf using Sacha Coppey sacha.coppey@oracle.com for the attribution?

Thank you! I sadly do not have commit rights, so please could someone commit this patch on my behalf using Sacha Coppey sacha.coppey@oracle.com for the attribution?

Sure - I'll do this later today

This revision was automatically updated to reflect the committed changes.