Page MenuHomePhabricator

[RISCV] Add GHC calling convention
ClosedPublic

Authored by schwab on Oct 20 2020, 5:17 AM.

Details

Summary

This is a special calling convention to be used by the GHC compiler.

Diff Detail

Event Timeline

schwab created this revision.Oct 20 2020, 5:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2020, 5:17 AM
schwab requested review of this revision.Oct 20 2020, 5:17 AM
schwab updated this revision to Diff 299391.Oct 20 2020, 9:44 AM
jrtc27 added inline comments.Oct 20 2020, 10:04 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2251

i32 works too, as with the fast calling convention. That way you can support RV32.

(Though maybe both should be asserting if they ever see an i64 on RV32 given that's not a legal type? Or just leave it up to the general legalisation/verification stuff)

2252

Do you have an explanation somewhere for why you've chosen these X and F registers for the calling convention? It'd also be useful to give their RISC-V ABI names here in the comments as the numbered registers aren't so easy to keep track of mentally.

2284

Shouldn't this be a hard error? Otherwise it'll fall back on the normal C calling convention like for Fast, no?

llvm/test/CodeGen/RISCV/ghc-cc.ll
1–3 ↗(On Diff #299391)

Please make this match the other tests in here, ie:

  • match the llc invocation style
  • test both rv32 and rv64
  • use update_llc_test_checks.py
lenary added inline comments.Oct 20 2020, 10:05 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2247–2286

Is this doable with the declarative Calling Convention system in LLVM (an example of this is in llvm/lib/Target/ARM/ARMCallingConv.td)

We haven't been able to do this for RISC-V's normal calling convention yet, but this convention seems a lot simpler, so I hope it should be doable.

jrtc27 added inline comments.Oct 20 2020, 10:06 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2247–2286

We deliberately went with C++ rather than TableGen for Fast, as it's far more readable (and flexible).

schwab added inline comments.Oct 20 2020, 10:25 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2284

This will error out in the caller CCState::AnalyzeCallOperands.

schwab added inline comments.Oct 20 2020, 10:28 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2252

I have just allocated the caller-saved registers in sequence s1 .. s11, fs0 .. fs11

schwab added inline comments.Oct 20 2020, 10:49 AM
llvm/test/CodeGen/RISCV/ghc-cc.ll
1–3 ↗(On Diff #299391)

How can I parameterize the types on the triple?

jrtc27 added inline comments.Oct 20 2020, 10:53 AM
llvm/test/CodeGen/RISCV/ghc-cc.ll
1–3 ↗(On Diff #299391)

Hm right you'd need i32. We pipe things through sed in our fork to s/\<iXLEN\>/i64/ etc but I don't think the upstream update_llc_test_checks.py likes that (though maybe it does these days?). You'd have to duplicate the file otherwise as is done for calling-conv-ilp32.ll and calling-conv-lp64.ll.

schwab updated this revision to Diff 300488.Oct 24 2020, 8:09 AM

Handle rv32 and testcase for rv32 added.

What happens if you try and use a soft-float or single-float ABI with ghccc? Will GHC know to use integer registers instead, or will this cause errors that mean we need to validate the ABI+CC combination?

llvm/test/CodeGen/RISCV/ghc-cc-rv32.ll
1 ↗(On Diff #300488)

Please use "ghccc" rather than "ghc-cc" in the names of these files to match the calling convention name (as is done with the fastcc tests).

1–2 ↗(On Diff #300488)

Please use update_llc_test_checks.py for these.

The --target-abi isn't actually needed, only -mattr=+f,+d.

schwab updated this revision to Diff 300499.Oct 24 2020, 11:55 AM

More error checking added

luismarques accepted this revision.Oct 28 2020, 4:59 PM

Overall, LGTM. Two things, though:

  1. Please fix the < %s issue before committing, otherwise the test will fail.
  2. Having a test for the error conditions would really round off this patch. I don't think that needs to be a blocker for merging this, but it would be nice.
llvm/test/CodeGen/RISCV/ghccc-rv32.ll
3

This is missing the < %s at the end of the llc command options.

This revision is now accepted and ready to land.Oct 28 2020, 4:59 PM
schwab updated this revision to Diff 301589.Oct 29 2020, 5:41 AM

Retested with typo fixed.

Could someone please commit this for me?

StephenFan added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2247–2286

In the CallingConvLower.h, the CCAssignFn is a function pointer that represent the function that deal with the calling convention, and this function can be used in many other places, for example, In globalisel's ValueHandler. So I think use the TableGen rather than C++ is more convenient.

jrtc27 added inline comments.Nov 19 2020, 6:26 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2247–2286

If GlobalISel can’t work with what’s already there then it needs to be fixed, because the current code is rather complex and won’t be being converted any time soon, and as a downstream consumer of the backend who extensively adds to the calling convention that would cause serious headache (and depending on how the rewriting is done may not even support our use case without additional downstream changes to TableGen, something that would be a real pain to do). IMO GlobalISel as a new framework needs to adapt to fit the existing backends unless there is a good reason why it fundamentally cannot.

Could someone please commit this for me?

Could someone please commit this for me?

I'll commit it. Thank you.

This revision was automatically updated to reflect the committed changes.

I'll commit it. Thank you.

The reason for me reverting the patch and re-applying it is that I had originally committed it without proper attribution. Sorry about that.