Page MenuHomePhabricator

[RISCV][WIP/RFC] Hard float ABI support
Needs ReviewPublic

Authored by asb on Apr 9 2019, 5:45 AM.

Details

Reviewers
rjmccall
Summary

The RISC-V hard float calling convention requires the frontend to:

  • Detect cases where, once "flattened", a struct can be passed using int+fp or fp+fp registers under the hard float ABI and coerce to the appropriate type(s)
  • Track usage of GPRs and FPRs in order to gate the above, and to determine when signext/zeroext attributes must be added to integer scalars

This patch attempts to do this in compliance with the documented ABI, and uses ABIArgInfo::CoerceAndExpand in order to do this. @rjmccall, as author of that code I've tagged you as reviewer for initial feedback on my usage.

Note that a previous version of the ABI indicated that when passing an int+fp struct using a GPR+FPR, the int would need to be sign or zero-extended appropriately. GCC never did this and the ABI was changed, which makes life easier as ABIArgInfo::CoerceAndExpand can't currently handle sign/zero-extension attributes.

I'm sharing just in case there are any concerns about this general approach. Known deficiencies in this patch are all related to testing:

  • Needs more basic ilp32d/lp64f/lp64d tests
  • Not enough coverage of bitfields in structs, packed+aligned structs etc

Diff Detail

Event Timeline

asb created this revision.Apr 9 2019, 5:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2019, 5:45 AM
rjmccall added inline comments.Apr 9 2019, 8:11 AM
lib/Basic/Targets/RISCV.h
102 ↗(On Diff #194295)

You can remove the TODO here, assuming that this CC support is all that's necessary to support these ABIs.

lib/CodeGen/TargetInfo.cpp
9223 ↗(On Diff #194295)

Should this include pointers? Pointers are often interchangeably with integers by ABIs.

The same question also applies to C++ references and data-member-pointer types, and maybe others that I'm not thinking of.

9298 ↗(On Diff #194295)

This definitely needs to handle bit-fields in some way.

9352 ↗(On Diff #194295)

This should use alignTo, not max. It's possible that they're equivalent for the narrow cases that can come out of all the checks above, but it's both clearer and safer to use the correct computation.

9374 ↗(On Diff #194295)

This seems like a reasonable use of coerce-and-expand.

asb updated this revision to Diff 198797.May 9 2019, 5:25 AM
asb marked 3 inline comments as done.

Update:

  • Expanded and improved tests
  • Set ABI defines
  • Remove errant TODO
  • Use alignTo

Still to do:

  • Review and test bitfield handling (which is likely incomplete)
lib/CodeGen/TargetInfo.cpp
9223 ↗(On Diff #194295)

The ABI doesn't consider pointers and integers interchangeable in this case. An "integer" is indeed an integer.

rjmccall added inline comments.May 16 2019, 10:32 AM
clang/lib/CodeGen/TargetInfo.cpp
9232

Is this the only consideration for floating-point types? Clang does have increasing support for half / various float16 types.

9236

The comment here is wrong because fp+fp is allowed, right?

Is this not already caught by the post-processing checks you do in detectFPCCEligibleStruct? Would it make more sense to just do all those checks there?

9258

Field2Ty = Field1Ty, please.

9288

I really expect there to be something in this block about whether the field is a bit-field. What exactly does your ABI specify if there's a bit-field?

mhorne added a subscriber: mhorne.Sat, Jun 1, 10:56 AM