This is an archive of the discontinued LLVM Phabricator instance.

[LegalizeTypes][RISCV] Support libcalls for fpto{s,u}i of bfloat by extending to f32 first
ClosedPublic

Authored by asb on Aug 3 2023, 5:06 AM.

Details

Summary

As there is no direct bf16 libcall for these conversions, extend to f32 first.

This patch includes a tiny refactoring to pull out equivalent logic in ExpandIntRes_XROUND_XRINT so it can be reused in ExpandIntRes_FP_TO_{S,U}INT.

This patch also demonstrates incorrect codegen for RV32 without zfbfmin for the newly enabled tests. As it doesn't introduce that incorrect codegen (caused by the assumption that 'TypeSoftPromoteHalf' is only used for f16 types), a fix will be added in a follow-up.

Diff Detail

Event Timeline

asb created this revision.Aug 3 2023, 5:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2023, 5:06 AM
asb requested review of this revision.Aug 3 2023, 5:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2023, 5:06 AM
bjope added a comment.Aug 3 2023, 11:13 AM

I'm not that familiar with floating point legalization. Is there a difference between using FP_EXTEND and for example F16_TO_FP/BF16_TO_FP (except that those only extend from a half precision type)?

Can you add some cases without zfbfmin?

joshua-arch1 added a comment.EditedAug 3 2023, 7:33 PM

Have you tried to directly modify some implementation in ExpandIntRes_FP_TO_{S,U}INT by adding another possibility of BF16_TO_FP when the FP type is TypeSoftPromoteHalf?

asb updated this revision to Diff 547791.Aug 7 2023, 7:49 AM
asb edited the summary of this revision. (Show Details)

@bjope: The F16_TO_FP and BF16_TO_FP nodes are slightly weird. As described in ISDOpcodes.h, they form a "semi-softened interface for dealing with bf16/f16 (as an i16)". I think it's correct to say they're only introduced when the LegalizeTypeAction for the type is TypeSoftPromoteHalf, which will never be set that way when bf16 is a legal type (i.e. when bf16 instructions are available). This patch is focused solely on that case, when those bf16 instructions are available. I don't think it would be correct to introduce BF16_TO_FP in this codepath (and if both were possible, I think the generic FP_EXT would be preferable given there are TODOs about trying to migrate F16_TO_FP and BF16_TO_FP to standard FP_EXT somehow).

@joshua-arch1: I've added CHECK lines for RV32/RV64 without zfbfmin. As you suggest, this does make an existing bug visible in the handling of bf16 on RV32 without zfbfmin caused by the assumption that TypeSoftPromoteHalf is only used for f16 types. That path won't be executed when bf16 is a legal type, as the type action for bf16 will never by TypeSoftPromoteHalf in that case. As this change merely includes a test case that exposes an existing bug rather than introducing it, I will post the fix in a separate patch.

This revision is now accepted and ready to land.Aug 7 2023, 8:25 PM