This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Custom lower FP_TO_FP16 and FP16_TO_FP to correct ABI of of libcall
ClosedPublic

Authored by asb on May 23 2023, 8:55 PM.

Details

Summary

As introduced in D99148, RISC-V uses the softPromoteHalf legalisation for fp16 values without zfh, with logic ensuring that f16 values are passed in lower bits of FPRs (see D98670) when F or D support is present. This legalisation produces ISD::FP_TO_FP16 and ISD::FP16_TO_FP nodes which (as described in ISDOpcodes.h) provide a "semi-softened interface for dealing with f16 (as an i16)". i.e. the return type of the FP_TO_FP16 is an integer rather than a float (and the arg of FP16_TO_FP is an integer). The remainder of the description focuses primarily on FP_TO_FP16 for ease of explanation.

FP_TO_FP16 is lowered to a libcall to __truncsfhf2 (float) or __truncdfhf2 (double). As of D92241, _Float16 is used as the return type of these libcalls if the host compiler accepts _Float16 in a test input (i.e. dst_t is set to _Float16). _Float16 is enabled for the RISC-V target as of D105001 and so the return value should be passed in an FPR on hard float ABIs.

This patch fixes the ABI issue in what appears to be a minimally invasive way - leaving the softPromoteHalf logic undisturbed, and lowering FP_TO_FP16 to an f32-returning libcall, converting its result to an XLen integer value. Alternate strategies might be possible involving custom legalisation of FP_ROUND with an f16 target.

As can be seen in the test changes, the custom lowering for FP16_TO_FP means the libcall is no longer tail-callable.

Outstanding issues:

  • Redundant fmv.x.w and fmv.w.x pairs produced in FP16_TO_FP lowering.
  • Missing STRICT variants.

Diff Detail

Event Timeline

asb created this revision.May 23 2023, 8:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2023, 8:55 PM
asb requested review of this revision.May 23 2023, 8:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2023, 8:55 PM
craig.topper edited the summary of this revision. (Show Details)May 24 2023, 10:28 AM
craig.topper edited the summary of this revision. (Show Details)
craig.topper edited the summary of this revision. (Show Details)

This seems like a good way to fix this to me. Custom lowering FP_ROUND wouldn't be enough. Soft promoting FADD and other arithmetic also creates FP16_TO_FP and FP_TO_FP16.

This revision is now accepted and ready to land.May 24 2023, 7:32 PM
kito-cheng accepted this revision.May 24 2023, 7:38 PM

LGTM from the ABI view

craig.topper added inline comments.May 30 2023, 4:53 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4927

I think we usually use the British spelling "legalisation"?

asb updated this revision to Diff 531240.Jun 14 2023, 2:41 AM
asb marked an inline comment as done.

Rebase, address review comment. I haven't had a chance yet to look at the strict opcodes or test related refactorings.

mr-c added a subscriber: mr-c.Jun 15 2023, 8:10 AM

Ping. I would like to see this get commited. We started noticing this ABI mismatch a few weeks ago so I've cherry-picked this to our downstream.

Hi, I have a question that why other targets don't have such custom expand code instead of default expand behavior?

asb edited the summary of this revision. (Show Details)Jun 30 2023, 8:43 AM
asb added a comment.Jun 30 2023, 8:49 AM

@zixuan-wu I would assume due to different ABI rules.

@craig.topper I've gone ahead and fixed what I think was the only real blocker to merging this (ensuring coverage in the existing half-convert.ll test rather than introducing new files) and committed.

zixuan-wu added inline comments.Jul 3 2023, 12:41 AM
llvm/test/CodeGen/RISCV/half-convert.ll
149 ↗(On Diff #536277)

why is there redundant fmv.x.w/fmv.w.x?

asb added inline comments.Jul 3 2023, 3:28 AM
llvm/test/CodeGen/RISCV/half-convert.ll
149 ↗(On Diff #536277)

We're presumably missing a combine or peephole to get rid of the redundant pair. This is a limitation noted in the patch description. As this patch is addressing a correctness issue, and the suboptimal code quality isn't disastrous, it focuses solely on fixing the incorrect codegen. A follow-on patch can address this code quality issue.

zixuan-wu added inline comments.Jul 3 2023, 6:52 PM
llvm/test/CodeGen/RISCV/half-convert.ll
149 ↗(On Diff #536277)

OK, sorry that I missed the description.

This comment was removed by joshua-arch1.