This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISel: Legalize s64->s16 G_SITOFP/G_UITOFP
ClosedPublic

Authored by Petar.Avramovic on Jul 15 2020, 8:22 AM.

Details

Summary

Add widenScalar for TypeIdx == 0 for G_SITOFP/G_UITOFP.
Legailize, using widenScalar, as s64->s32 G_SITOFP/G_UITOFP
followed by s32->s16 G_FPTRUNC.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2020, 8:22 AM
foad added a comment.Jul 15 2020, 8:26 AM

Legailize as s64->s32 G_SITOFP/G_UITOFP followed by s32->s16 G_FPTRUNC.

It seems like this would give the wrong result in some cases because of the double rounding. What does SelectionDAG do?

Legailize as s64->s32 G_SITOFP/G_UITOFP followed by s32->s16 G_FPTRUNC.

It seems like this would give the wrong result in some cases because of the double rounding. What does SelectionDAG do?

It looks like it does the same thing. AMDGPU and AArch64 both seem to do the same thing in custom lowering. I think since the source is an integer, there really isn't a whole lot of opportunity for rounding to do much.

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
647

Could you move this to widenScalar for TypeIdx == 0 in LegalizerHelper instead of making this custom?

1821

Missing spaces around ==

foad added a comment.Jul 15 2020, 9:20 AM

Legailize as s64->s32 G_SITOFP/G_UITOFP followed by s32->s16 G_FPTRUNC.

It seems like this would give the wrong result in some cases because of the double rounding.

The range of a half is only about +/- 65536, and any integer in that range will convert to float exactly without rounding, so there is no double rounding problem.

Petar.Avramovic edited the summary of this revision. (Show Details)
arsenm accepted this revision.Jul 16 2020, 5:43 AM

LGTM

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-uitofp.mir
484

Can also use v2s64 to v2s16

This revision is now accepted and ready to land.Jul 16 2020, 5:43 AM
This revision was automatically updated to reflect the committed changes.