This is an archive of the discontinued LLVM Phabricator instance.

[LegalizeTypes][RISCV] Correct FP_TO_{S,U}INT expansion when bf16 isn't a legal type
ClosedPublic

Authored by asb on Aug 7 2023, 7:52 AM.

Details

Summary

As noted in D156990, the logic in ExpandIntRes_FP_TO_SINT assumes that if the type action for the float type is TypeSoftPromoteHalf, is must have been an f16 (half). However, the meaning of that type action has been overloaded and it is used for both f16 and bf16. This patch adds an appropriate check to ensure ISD::FP16_TO_FP or ISD::BF16_TO_FP is emitted as required.

Diff Detail

Event Timeline

asb created this revision.Aug 7 2023, 7:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2023, 7:52 AM
asb requested review of this revision.Aug 7 2023, 7:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2023, 7:52 AM
asb edited the summary of this revision. (Show Details)Aug 7 2023, 8:14 AM
asb updated this revision to Diff 548179.Aug 8 2023, 6:02 AM
  1. Rebase (all dependent patches are now committed)
  2. Remove TODO comments that are fixed by this patch
asb updated this revision to Diff 548189.Aug 8 2023, 6:26 AM

Apologies for the noise, the previous versions had a bug in that they were checking the type of Op.getValueType after Op = GetSoftPromotedHalf(Op). The patch now correctly gets the old FP VT before doing that call. This bug was caught by existing tests (I just hadn't seen that half-convert.ll failed) so no further test changes needed.

joshua-arch1 added a comment.EditedAug 9 2023, 1:34 AM

LGTM. That's exactly what I meant to do when commenting in D156990.

joshua-arch1 accepted this revision.Aug 9 2023, 1:35 AM
This revision is now accepted and ready to land.Aug 9 2023, 1:35 AM