This is an archive of the discontinued LLVM Phabricator instance.

[mips][msa] Implement f16 support
ClosedPublic

Authored by sdardis on Nov 8 2016, 7:31 AM.

Details

Summary

The MIPS MSA ASE provides instructions to convert to and from half precision
floating point. This patch teaches the MIPS backend to treat f16 as a legal
type and how to promote such values to f32 for the usual set of operations.

As a result of this, the fexup[lr].w intrinsics no longer crash LLVM during
type legalization.

Diff Detail

Repository
rL LLVM

Event Timeline

sdardis updated this revision to Diff 77192.Nov 8 2016, 7:31 AM
sdardis retitled this revision from to [mips][msa] Implement f16 support.
sdardis updated this object.
sdardis added a reviewer: vkalintiris.
sdardis set the repository for this revision to rL LLVM.
sdardis added a subscriber: llvm-commits.
zoran.jovanovic added inline comments.
lib/Target/Mips/MipsMSAInstrInfo.td
3747 ↗(On Diff #77192)

Nit: Next two lines exceed 80 characters.

3752 ↗(On Diff #77192)

Nit: Next two lines exceed 80 characters.

3757 ↗(On Diff #77192)

Nit: Next two lines exceed 80 characters.

3762 ↗(On Diff #77192)

Nit: Next two lines exceed 80 characters.

lib/Target/Mips/MipsSEISelLowering.cpp
3448 ↗(On Diff #77192)

Nit: I do believe that Mips::GPR64RegClass and 64-bit variants of instructions should be used with N32 too.
Same is for code in MipsSETargetLowering::emitLD_F16_PSEUDO.

3655 ↗(On Diff #77192)

Nit: Is this comment correct?
From the code below (and corresponding test cases) it seems that fexupr.d instruction should be generated after this one.

3689 ↗(On Diff #77192)

if comment above is correct IsFGR64 should be replaced with IsFGR64onMips64.

zoran.jovanovic accepted this revision.Nov 16 2016, 7:34 AM
zoran.jovanovic added a reviewer: zoran.jovanovic.
zoran.jovanovic edited edge metadata.

LGTM otherwise.

This revision is now accepted and ready to land.Nov 16 2016, 7:34 AM
sdardis planned changes to this revision.Nov 17 2016, 2:39 AM
sdardis marked 6 inline comments as done.

As I mention in my inline comments, I'm revising this patch. I'll highlight the differences when I repost.

lib/Target/Mips/MipsSEISelLowering.cpp
3448 ↗(On Diff #77192)

Looking at this, I have found test cases where N32 requires either GPR32RegClass or GPR64RegClass depending on the context. I'll post a revised version of this patch soon. I've also found 1 or 2 places where the wrong MSA regclass is used.

3655 ↗(On Diff #77192)

Yes, you're correct, I missed that when writing the comment. Probably a copy/paste thing.

3689 ↗(On Diff #77192)

(For completeness' sake) The comment above is wrong, If we're expanding an f16 to FPR64, we need FEXUPRD as well. The case of Mips32/Mips64 doesn't apply here.

sdardis updated this revision to Diff 78347.Nov 17 2016, 4:52 AM
sdardis edited edge metadata.
sdardis removed rL LLVM as the repository for this revision.
sdardis marked 2 inline comments as done.

Updated the handling of loads and stores for f16 to better match the register class that is actually used. Added test coverage for that area. We're still unable to enable the machine verifier for this, as some MSA instructions are missing their 64 bit counterparts (fill, copy_[su].[bhwd] relating to this patch).

This revision is now accepted and ready to land.Nov 17 2016, 4:52 AM
sdardis updated this revision to Diff 78350.Nov 17 2016, 4:59 AM
sdardis edited edge metadata.

Fix usage of MSA register class for f16 to FGR64 expansion.

sdardis added inline comments.Nov 17 2016, 5:05 AM
lib/Target/Mips/MipsSEISelLowering.cpp
3448 ↗(On Diff #77192)

I've updated this use the register class of the operand if it's a register, otherwise use GPR64 if it's not O32. Likewise for LD_F16.

test/CodeGen/Mips/msa/f16-llvm-ir.ll
24–62 ↗(On Diff #78350)

New test coverage for testing frame indexed accesses.

These changes OK?

This revision was automatically updated to reflect the committed changes.

For cross referencing purposes, the tests included in the first diff should have been part of the last diff. They have been committed in rL287574.