This is an archive of the discontinued LLVM Phabricator instance.

Add support to promote f16 to f32
ClosedPublic

Authored by pirama on Mar 31 2015, 4:35 PM.

Details

Summary

This patch adds legalization support to operate on FP16 as a load/store type
and do operations on it as floats.

Generic pass/fail tests have been added to
test/CodeGen/Generic/fp16-promote.ll.

Diff Detail

Repository
rL LLVM

Event Timeline

pirama updated this revision to Diff 23019.Mar 31 2015, 4:35 PM
pirama retitled this revision from to Add support to promote f16 to f32.
pirama updated this object.
pirama edited the test plan for this revision. (Show Details)
pirama added reviewers: srhines, t.p.northover.

This patch is rough on the edges and needs to handle:

  1. the right approach to expose this as a command-line option. Right now, the option is added to TargetOptions.h, but that might not be right.
  2. I do not have a reasonable way of finding whether ISD::FP_TO_FP16, ISD::FP16_TO_FP are supported by the target. If unsupported, we have to fall back to making a libcall to gnu_h2f_ieee. Right now, this is a hard-coded branch in LegalizeFloatTypes.cpp:GetPromotedValue but I need suggestions on how to handle this.
  3. Once the high-level issues are ironed out, I'll add target-specific tests to verify instructions for this promotion.
pirama added a subscriber: Unknown Object (MLST).Mar 31 2015, 4:43 PM

I forgot to subscribe llvm-commits. So, I'm re-sending the Summary and the raw diff to the mailing list.

Summary:
This patch adds legalization support to operate on FP16 as a load/store type
and do operations on it as floats.

Generic pass/fail tests have been added to
test/CodeGen/Generic/fp16-promote.ll.

This patch is rough on the edges and needs to handle:

the right approach to expose this as a command-line option. Right now, the option is added to TargetOptions.h, but that might not be right.
I do not have a reasonable way of finding whether ISD::FP_TO_FP16, ISD::FP16_TO_FP are supported by the target. If unsupported, we have to fall back to making a libcall to gnu_h2f_ieee. Right now, this is a hard-coded branch in LegalizeFloatTypes.cpp:GetPromotedValue but I need suggestions on how to handle this.
Once the high-level issues are ironed out, I'll add target-specific tests to verify instructions for this promotion.

ab added a reviewer: ab.Apr 1 2015, 10:34 AM
ab added a subscriber: ab.

Awesome, I wanted to do this next, thanks for working on this =)

I'll look into this closer, but a couple of very high level not-really-questions first:

  • what about other types? I just removed the generic f32->f64 promotion a few days ago (in anticipation for such a PromoteFloat legalizer). I'm not sure we can test that in-tree, so there's probably not much point in worrying about this.
  • might I ask: what's your use case? In particular, do you intend to change clang in any way?

In the meantime, you might be interested in http://reviews.llvm.org/D8648 for tests, as I tried to cover as many IR/generic ISD opcodes as possible. The patch itself just focuses on ops promotion on AArch64 (where half is legal).

-Ahmed

ab added inline comments.Apr 1 2015, 3:56 PM
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
1591 ↗(On Diff #23019)

I don't think you need this, just always pick some opcode, and let the operation legalizer decide it doesn't like a type combination.

1596 ↗(On Diff #23019)

'else' with '{'. Same elsewhere

1604 ↗(On Diff #23019)

llvm_unreachable, or better still, report_fatal_error, as this is actually reachable from purpose-crafted IR? Same below.

1608 ↗(On Diff #23019)

As I said on the ML, not sure if it'll work, but you should just let the ops legalizer pick a libcall for the FP_TO_FP nodes.

1626 ↗(On Diff #23019)

I didn't notice this, and found the name a bit confusing, thinking this was some magic legalizer method. Might be good to have a more descriptive name, making it explicit we actually promote here, rather than get a previously promoted value as GetPromotedInt/Float does.

The fact that it goes both ways is also a bit confusing, but I get that avoids duplication. For such a short function, it might be worth doing two separate versions, perhaps?

1700 ↗(On Diff #23019)

I'm not sure what to think of this. Could we catch this in a DAGCombine instead?

1701 ↗(On Diff #23019)

'Op->getOpcode()' instead, as you already do for getValueType. Same elsewhere

1704 ↗(On Diff #23019)

Dead

1753 ↗(On Diff #23019)

Should these optimizations be DAGCombines instead?

1804 ↗(On Diff #23019)

What about it? :P

1809 ↗(On Diff #23019)

This isn't a binop.

1982 ↗(On Diff #23019)

I don't follow. This is about rounding to integers, and has nothing to do with FP, right? Why not simply GetPromotedFloat? Or, for that matter, just reusing PromoteFloatRes_UnaryOp.

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
260 ↗(On Diff #23019)

The line break seems odd, should there be more in the first line?

262 ↗(On Diff #23019)

Whether the fallthrough is intended or not, I think an explicit 'break' or comment would be useful.

lib/CodeGen/SelectionDAG/LegalizeTypes.h
512 ↗(On Diff #23019)

Move this to the .cpp?

pirama added a comment.Apr 1 2015, 5:25 PM

Thanks for the code review ab. I'll make your suggestions, as well as remove the command line option.

Looks like the Mips backend isn't setting the right operation action for FP_TO_FP16. I'll fix that in a different patch.

pirama added inline comments.Apr 2 2015, 1:38 PM
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
1604 ↗(On Diff #23019)

I'll switch to report_fatal_error.

1608 ↗(On Diff #23019)

http://reviews.llvm.org/D8804 should let the Op legalizer do the right thing for Mips.

1626 ↗(On Diff #23019)

I'll remove this function and directly use DAG.getNode at its callers - since libcalls are no longer handled here.

1700 ↗(On Diff #23019)

I'll attempt to do this in the DAGCombiner.

1982 ↗(On Diff #23019)

I couldn't find any documentation or reference for this opcode code. So, I assumed it was similar to float rounding, while at the same time wondering the difference between FROUND and FTRUNC. I'll reuse PromoteFloatRes_UnaryOp for FTRUNC.

The above function still applies to FROUND. I am trying to trim the extra precision as this node be an explicit operation to reduce precision.

lib/CodeGen/SelectionDAG/LegalizeTypes.h
512 ↗(On Diff #23019)

I'll leave it here to be consistent with the other GetPromotedInteger and similar functions in this file.

ab added inline comments.Apr 2 2015, 2:10 PM
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
1982 ↗(On Diff #23019)

So, FP_ROUND is the opcode used to do fp->fp rounding (i.e., trim the extra precision). FTRUNC and FROUND are equivalent to the trunc and round libm functions, i.e., rounding to integer. IIRC the difference is that one rounds to infinity, and the other to zero. (-0.1 -> -1 vs 0).

lib/CodeGen/SelectionDAG/LegalizeTypes.h
512 ↗(On Diff #23019)

Fair enough, I asked because I scrolled up a bit to see whether the others did the same. I saw GetExpandedFloat was in the .cpp, but I was too quick to assume they would be consistent. Either way I'm fine; a follow-up patch to put them consistently in one or the other would be nice.

pirama updated this revision to Diff 23435.Apr 8 2015, 12:48 PM
  • Removed the command line option and instead enable PromoteFloat on targets where f32 is legal
  • Updated LegalizeFloatTypes.cpp covering ab's comments.
  • Added a comprehensive test of fp16 promotion to test/Codegen/ARM/fp16-promote.ll
pirama added a comment.Apr 8 2015, 1:38 PM

In ARM-32 ABI, f16 args and return values gets promoted to f32. To keep calling convention out of the picture, the ARM codegen tests take half* args to load arguments and store results.

ab edited edge metadata.Apr 13 2015, 1:40 PM

Looking better, thanks!

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
1817 ↗(On Diff #23435)

This needs tests (also for insert_vector_elt ?)

1932–1933 ↗(On Diff #23435)

Stale reference to FROUND?

test/CodeGen/ARM/fp16-promote.ll
1 ↗(On Diff #23435)

We should also have tests for non-native conversions with the libcall.

test/CodeGen/Generic/fp16-promote.ll
1 ↗(On Diff #23435)

Some of the tests have half parameters and return types. I'm not sure that's expected to work on all targets, is it?

If it's not, I still see value in a generic sanity check like this, so dealing with pointers instead is probably fine.

ab added inline comments.Apr 13 2015, 1:48 PM
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
1817 ↗(On Diff #23435)

Which makes me think: I haven't tested structs in D8648, but they have a few interesting operations that we should also deal with, mostly insert/extract element. By-value returns/arguments have the same problems as scalar returns/arguments; making sure the call lowering handles them fine is still useful (as you do for scalar f16).

test/CodeGen/ARM/fp16-promote.ll
127–128 ↗(On Diff #23435)

This could still use some CHECK-lines, if only to demonstrate they are indeed handled as f32.

pirama added inline comments.Apr 14 2015, 10:34 AM
test/CodeGen/Generic/fp16-promote.ll
1 ↗(On Diff #23435)

I agree that the tests here should take pointers.

I actually uploaded this as a placeholder for my initial patch. If it's ok, I'll delete the Generic tests in my next upload, and add them later. I need to commit the Mips patch (http://reviews.llvm.org/D8804) and upload a similar one for X86 before the tests will pass.

pirama updated this revision to Diff 23747.Apr 14 2015, 3:46 PM
pirama edited edge metadata.
  • Removed test/Generic to be added at a later time. Some backends need patches before they can pass these tests.
  • Added tests for libcall conversions
  • Added insert_vector_elt and extract_vector_elt tests
  • Added struct tests (pass-by-value, return-by-value, extractvalue, insertvalue)
  • Added CHECKs for half args and returns
ab accepted this revision.Apr 17 2015, 10:18 AM
ab edited edge metadata.

I don't see anything missing; LGTM with a couple nits.

Thanks!

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
1595 ↗(On Diff #23747)

report_fatal_error?

test/CodeGen/ARM/fp16-promote.ll
1164 ↗(On Diff #23747)

tored -> stored

This revision is now accepted and ready to land.Apr 17 2015, 10:18 AM
ab added inline comments.Apr 17 2015, 10:19 AM
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
1701 ↗(On Diff #23747)

dl -> DL ? (here and elsewhere)

pirama updated this revision to Diff 23948.Apr 17 2015, 10:52 AM
pirama edited edge metadata.

Thanks for the review and LGTM, ab.

  • Minor changes based on prior comments
  • There are a few instances of 'dl' in the softening and expansion routines in the same file. That can be cleaned up as a separate patch.
pirama updated this revision to Diff 23951.Apr 17 2015, 11:21 AM

Minor fixups. Remove a todo that had no functional impact.

srhines accepted this revision.Apr 17 2015, 11:35 AM
srhines edited edge metadata.
This revision was automatically updated to reflect the committed changes.