Page MenuHomePhabricator

[MIPS] OperationAction for FP_TO_FP16, FP16_TO_FP
ClosedPublic

Authored by pirama on Apr 2 2015, 10:44 AM.

Details

Summary

Set operation action for FP16 conversion opcodes, so the Op legalizer
can choose the gnu_* libcalls for Mips.

Diff Detail

Repository
rL LLVM

Event Timeline

pirama updated this revision to Diff 23167.Apr 2 2015, 10:44 AM
pirama retitled this revision from to [MIPS] OperationAction for FP_TO_FP16, FP16_TO_FP.
pirama updated this object.
pirama edited the test plan for this revision. (Show Details)
pirama added a reviewer: srhines.
pirama added subscribers: ab, Unknown Object (MLST).
pirama updated this revision to Diff 23168.Apr 2 2015, 10:47 AM

[MIPS] OperationAction for FP_TO_FP16, FP16_TO_FP

Set operation action for FP16 conversion opcodes, so the Op legalizer
can choose the gnu_* libcalls for Mips.

This is related to http://reviews.llvm.org/D8755, which adds support to consider FP16 types as load/store-only types.

ab added a comment.Apr 2 2015, 1:58 PM

Do you need this for f64 as well? Also, please add some tests, to make sure we actually get to the libcalls.

dsanders edited edge metadata.Apr 7 2015, 3:31 AM

Thanks for the patch and sorry for the delay in looking at it (we've just had two public holidays in the UK). The change looks right to me for f32 but as Ahmed says it needs a test case checking that it emits the call to the library.

Ok. I'll do that after I land http://reviews.llvm.org/D8755 since I'd need that to write tests for this patch.

pirama updated this revision to Diff 23943.EditedApr 17 2015, 10:24 AM
pirama edited edge metadata.
  • Added loadext and truncstore actions for f16
  • Added tests to ensure that libcalls get inserted
dsanders accepted this revision.Apr 20 2015, 8:56 AM
dsanders edited edge metadata.

LGTM with a small nit in the test.

lib/Target/Mips/MipsISelLowering.cpp
226–229 ↗(On Diff #23943)

While iterating, this will do 3 strange calls:

setLoadExtAction(ISD::EXTLOAD, MVT::f16, MVT::f32, Expand);
setLoadExtAction(ISD::EXTLOAD, MVT::f16, MVT::f16, Expand);
setLoadExtAction(ISD::EXTLOAD, MVT::f32, MVT::f32, Expand);

I assume it isn't a problem to do this since EXTLOAD doesn't make sense when the memory type is larger (or the same as) the value type. It might be better to list the valid cases like we do for setTruncStoreAction() below. I don't mind which approach is taken.

test/CodeGen/Mips/fp16-promote.ll
3–5 ↗(On Diff #23943)

Nit: These two lines aren't necessary and it's best not to duplicate the datalayout in the test in case we ever change it. The bit I think might change at some point is the vector part since our vectors only require the elements to be aligned and not the whole vector.

64–71 ↗(On Diff #23943)

Just a comment, no change required: This might be a little fragile (the exact order of the output may vary) but I don't see a good way to make it more robust at the moment.

This revision is now accepted and ready to land.Apr 20 2015, 8:56 AM
pirama updated this revision to Diff 24036.Apr 20 2015, 11:30 AM
pirama edited edge metadata.

Remove datalayout from the tests, and use CHECK-DAG for fpext_double test.

lib/Target/Mips/MipsISelLowering.cpp
226–229 ↗(On Diff #23943)

fp_valuetypes has larger FP types (f80, f128 etc). Enumerating all of them will be tedious and skipping these types is a divergence from current behavior.

The only strange call introduced by this patch is the second one you listed. Such an EXTLOAD should never be generated, so I'll leave this as is.

test/CodeGen/Mips/fp16-promote.ll
3–5 ↗(On Diff #23943)

Done

64–71 ↗(On Diff #23943)

I'll switch to CHECK-DAG for this test. It's still not robust enough, but is better than the current state.

This revision was automatically updated to reflect the committed changes.

In future, please don't commit after a conditional LGTM if the conditions haven't been met. If you disagree with the conditions, you should discuss them further.

lib/Target/Mips/MipsISelLowering.cpp
226–229 ↗(On Diff #23943)

fp_valuetypes has larger FP types (f80, f128 etc). Enumerating all of them will be tedious and skipping these types is a divergence from current behavior.

Good point. Iterating makes sense in that case.

The only strange call introduced by this patch is the second one you listed. Such an EXTLOAD should never be generated, so I'll leave this as is.

The first one is odd because it 'extends' from a f32 to a f16 which is actually a truncation operation.
The third one is odd for the same reason as the second one. It 'extends' to the same type as in memory.

I do agree that such EXTLOAD's shouldn't be generated though.

test/CodeGen/Mips/fp16-promote.ll
64–71 ↗(On Diff #23943)

I didn't suggest CHECK-DAG here because it doesn't do what you'd expect when the match strings are the same. All the 'CHECK-LIBCALL-DAG: cvt.d.s' will match at the same position in the file (the first instance of 'cvt.d.s'). This is because the search for CHECK-DAG matches are all matched (starting from the same position in the file) before ordering violations are determined.

For example, the following file will pass because both CHECK-DAG's match the same 'foo'.

; CHECK-DAG: fo{{o}}
; CHECK-DAG: fo{{o}}
foo

In future, please don't commit after a conditional LGTM if the conditions haven't been met. If you disagree with the conditions, you should discuss them further.

My apologies Daniel. I assumed an "Accepted" status in Phabricator carries over across minor updates, and that my last patch was minor. I was wrong on both counts. In the future, I'll wait for a LGTM on all patches.

I'll address the CHECK-DAG concern in a different patch and link it back to here.

test/CodeGen/Mips/fp16-promote.ll
64–71 ↗(On Diff #23943)

You are right. In fact, I found out about this in a different context, but missed it here. I think the right thing to do here is to change CHECK-DAG to CHECK and add a comment noting the fragility of this test.

No worries. It turned out that I agreed with most of your replies anyway :-).

I'll address the CHECK-DAG concern in a different patch and link it back to
here.

Sounds good to me. Thanks.