Page MenuHomePhabricator

[RISCV] Add custom CC_RISCV calling convention and improved call support
ClosedPublic

Authored by asb on Nov 10 2017, 5:21 AM.

Details

Summary

As a reminder, RISC-V calling conventions can be reviewed at https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#procedure-calling-convention. You may also find my "golden model" useful https://github.com/lowRISC/riscv-calling-conv-model.

After lots of back and forth, I'm ended up implementing a custom calling convention function. I've found TableGen-based calling convention definitions to lack flexibility and require substantial boilerplate when you reach their limitations (see e.g. MipsCCState). I'm open to alternative suggestions of course. An additional advantage of CC_RISCV is that it can be trivially parametrised by XLEN and FLEN, meaning the same code paths can be used to support the 10 RISC-V calling conventions we need to support (-mabi=ilp32e, -mabi=ilp32, -mabi=ilp32f, -mabil=ilp32d, rv32i* varargs, rv32e* varargs, -mabi=lp64, -mabi=lp64f, -mabi=lp64d, rv64 varargs).

This patch adds support for:

  • Passing large scalars according to the RV32I calling convention
  • Byval arguments
  • Passing values on the stack when the argument registers are exhausted

It also documents the ABI lowering that a language frontend is expected to perform.

A follow-up patch adds support for varargs.

Although I've kept it as a separate patch at https://github.com/lowrisc/riscv-llvm, I've included changes to CallingConvLower.h here in order to make review easier. We add PendingArgFlags CCState, as a companion to PendingLocs.

The PendingLocs vector is used by a number of backends to handle arguments that are split during legalisation. However CCValAssign doesn't keep track of the original argument alignment. Therefore, add a PendingArgFlags vector which can be used to keep track of the ISD::ArgFlagsTy for every value added to PendingLocs.

CCing @kparzysz. As another backend owner who has also ended up implementing custom calling convention functions I thought you may also have input.

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.Nov 10 2017, 5:21 AM

Some minor reviews. I will look at the test cases tomorrow to check if we cover most cases.

lib/Target/RISCV/RISCVISelLowering.cpp
389 ↗(On Diff #122422)
398 ↗(On Diff #122422)

function description?

406 ↗(On Diff #122422)

nit: don't we use "llvm_unreachable" for yet to be implemented logic?

417 ↗(On Diff #122422)

This seems like something CCStat need to worry about. Why are we asserting for this here? plus, shouldn't we add a message describing the assertion if and when it fails?

435 ↗(On Diff #122422)

assert description?

451 ↗(On Diff #122422)

be passed indirect

passed indirectly?

453 ↗(On Diff #122422)

assert description?

asb updated this revision to Diff 122628.Nov 13 2017, 2:54 AM
asb marked 4 inline comments as done.

Updated patch to address review comments (thanks!).

asb added inline comments.Nov 13 2017, 2:54 AM
lib/Target/RISCV/RISCVISelLowering.cpp
389 ↗(On Diff #122422)

It will. Stack alignment is handled by RISCVFrameLowering::determineFrameLayout and RISCVFrameLowering::emitPrologue. callee_many_scalars and caller_many_scalars in test/CodeGen/RISCV/calling-conv.ll test this code path.

406 ↗(On Diff #122422)

No, llvm_unreachable is primarily useful for unreachable code (e.g. default statements in a switch). assert(IsFixed && "Vararg support not yet implemented"); is preferable to:

if (IsFixed)
  llvm_unreachable("Vararg support not yet implemented");
417 ↗(On Diff #122422)

It would require a refactoring of CCState and likely a modification of other CCState users to move the assert there.

asb updated this revision to Diff 122629.Nov 13 2017, 3:04 AM

Fix typo.

sameer.abuasal edited reviewers, added: sabuasal; removed: sameer.abuasal.Nov 13 2017, 11:45 AM
mgrang added inline comments.Nov 20 2017, 4:16 PM
lib/Target/RISCV/RISCVISelLowering.cpp
659 ↗(On Diff #122629)

Period after comment.

824 ↗(On Diff #122629)

Any reason to have to lowercase loop variables in some functions and uppercase in some others? Can we make this uniform?

asb updated this revision to Diff 123767.Nov 21 2017, 4:57 AM
asb marked 2 inline comments as done.

Rebase and address review comments.

Grepping the LLVM codebase, it seems that lowercase single-letter variable names is most common for simple integer for-loop counter variables, so I've standardised on this.

mgrang accepted this revision.Nov 21 2017, 10:54 AM

LGTM.

This revision is now accepted and ready to land.Nov 21 2017, 10:54 AM
mgrang added inline comments.Nov 21 2017, 11:54 AM
lib/Target/RISCV/RISCVISelLowering.cpp
416 ↗(On Diff #123767)

@asb I do not see the definition of getPendingArgFlags in CallingConvLower.h and it gives me a build error.

error: no member named 'getPendingArgFlags' in 'llvm::CCState'
      State.getPendingArgFlags();
asb updated this revision to Diff 123833.Nov 21 2017, 12:01 PM

@mgrang: Apologies, that hunk was accidentally dropped when I refreshed the patch. Updated to include the CallingConvLower.h changes.

mgrang added inline comments.Nov 21 2017, 12:20 PM
lib/Target/RISCV/RISCVISelLowering.cpp
416 ↗(On Diff #123767)

I have fixed this in D40318.

In D39898#932050, @asb wrote:

@mgrang: Apologies, that hunk was accidentally dropped when I refreshed the patch. Updated to include the CallingConvLower.h changes.

Thanks for fixing this here. I can abandon D40318.

apazos added inline comments.Nov 22 2017, 10:12 AM
lib/Target/RISCV/RISCVISelLowering.cpp
491 ↗(On Diff #123833)

add a comment next to true to indicate the var being set

502 ↗(On Diff #123833)

CLI argument does not seem to be used in this function.

apazos added inline comments.Nov 22 2017, 10:47 AM
lib/Target/RISCV/RISCVISelLowering.cpp
342 ↗(On Diff #123833)

capitalize xlen as it is referred as XLEN in other places

343 ↗(On Diff #123833)

end point missing.

345 ↗(On Diff #123833)

xlen -> XLEN

360 ↗(On Diff #123833)

2xlen -> 2x XLEN? Shound'nt we agree on a convention?

386 ↗(On Diff #123833)

Maybe if you add Reg1 Reg2 vars upfront, then you can rewrite the logic in a simpler way?

if (Reg1) {
if (Reg2)

// both halves in regs

else

// first half in reg only, the other on the stack

} else

// both halves on the stack
414 ↗(On Diff #123833)

why change the name from PendingLocs to PendingMembers?

asb updated this revision to Diff 125381.Dec 4 2017, 11:31 AM
asb marked 7 inline comments as done.

Update to address review comments.

lib/Target/RISCV/RISCVISelLowering.cpp
386 ↗(On Diff #123833)

Doesn't this way require more repetition? e.g. the logic to pass the first half via registers must be replicated in case 1 and 2, and the logic to pass the second half on the stack must be replicated in case 2 and case 3.

414 ↗(On Diff #123833)

Honestly, because all other backends accessing State.getPendingLocs name the result PendingMembers, and I assumed the name made sense to others. I don't see a good reason for the name, so I've gone ahead and used PendingLocs instead.

asb updated this revision to Diff 125384.Dec 4 2017, 11:46 AM

Add comments indicating the meaning of true/false arguments in more cases.

asb updated this revision to Diff 126147.Dec 8 2017, 7:10 AM

Rebase patch.

This revision was automatically updated to reflect the committed changes.