This is an archive of the discontinued LLVM Phabricator instance.

Always promote f16
ClosedPublic

Authored by olista01 on Sep 14 2015, 11:44 AM.

Details

Summary

Bug: https://llvm.org/bugs/show_bug.cgi?id=24549

  • Softening support of f16 was never finished, really. Always promote f16 to f32. If f32 is not legal, rely on f32 legalization after promotion.
  • Update SoftenFloatRes_FP_EXTEND to look for a promoted operand if the operand's type action is to promote.
  • Add the test case from the linked bug.

Ideally, we should test all the cases in
test/CodeGen/ARM/fp16-promote.ll with -vfp2. Doing that needs extra
work:

  • some functions in this test don't compile
  • all the functions need a new set of CHECKs for when both f32 and f16 are illegal.

Diff Detail

Event Timeline

pirama updated this revision to Diff 34717.Sep 14 2015, 11:44 AM
pirama retitled this revision from to Always promote f16.
pirama updated this object.
pirama added reviewers: ab, olista01, srhines.
pirama added a subscriber: llvm-commits.

Created https://llvm.org/bugs/show_bug.cgi?id=24814 to track testing of fp16-promote.ll with -vfp2.

ab added inline comments.Sep 22 2015, 9:41 AM
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
424–425

IIUC, this means we'll do something like:

(f32 (fpext (f16 ...)))
(i32 (fpext (f32 <promoted f16 ...>)))
(i32 (__gnu_h2f_ieee (f32 ...)))
(i32 (__gnu_h2f_ieee (i32 ...)))

even though __gnu_h2f_ieee expects uint16_t. On ARM uint16_t will get promoted to i32, but now we're interpreting a full f32-softened-to-i32 as an i16-promoted-to-i32, no?

Doesn't this mean it's possible to get stuff like:

(i32 (__gnu_h2f_ieee
  (i32 (__gnu_h2f_ieee
    (i32 (load zext from i16 ...))
  ))
))

by legalizing:

(f32 (fpext (f16 (load ...))))

?

pirama added inline comments.Sep 23 2015, 10:02 PM
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
424–425

You are right. I completely missed testing this. Also, Op isn't legalized yet at this point. I'll fix this and upload a new revision.

olista01 edited edge metadata.Oct 29 2015, 6:22 AM

Hi Pirama,

Are you still working on this? One of our internal tests suites is currently failing for fp16, and this looks like it should fix it (except for the FPEXT issue Ahmed spotted).

Oliver

Hi Oliver, I started on a different activity and completely forgot about this. Hopefully, I'll have some time late next week to look into this. Please feel free to take over this patch if you have the cycles to fix the FPEXT issue.

Thanks,
-Pirama

olista01 commandeered this revision.Oct 30 2015, 10:33 AM
olista01 edited reviewers, added: pirama; removed: olista01.
olista01 updated this revision to Diff 38824.Oct 30 2015, 10:36 AM

Updated to correctly handle FPEXT, and get test/CodeGen/ARM/fp16-promote.ll passing. The changes in SelectionDAGBuilder.cpp were required for a few of the test cases, notably those with half-precision arguments (which the clang frontend doesn't currently emit, at least for ARM), and phi nodes.

ab accepted this revision.Nov 6 2015, 4:34 PM
ab edited edge metadata.

I think this is fine, modulo a couple nits.

Pirama: any comments?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
201 ↗(On Diff #38824)

&& bitsLT ?

getNode() is pretty involved, and here we'd end up calling it to insert a no-op truncate for every softened f32/f64, right?

test/CodeGen/ARM/fp16-promote.ll
153–156 ↗(On Diff #38824)

-LIBCALL/-ALL ?

885–886 ↗(On Diff #38824)

Did we start emitting anything else? If not, I'd keep the -NEXT and .fnstart.

This revision is now accepted and ready to land.Nov 6 2015, 4:34 PM
pirama accepted this revision.Nov 7 2015, 7:17 PM
pirama edited edge metadata.

LGTM, after ab's changes. Thanks for taking over this patch and also updating the test. Can you also clean up the commit message, now that you updated the test? (Or did you already update it and it is just not visible in Phabricator?)

This revision was automatically updated to reflect the committed changes.

Thanks, committed revision 252459 with the suggested changes.

test/CodeGen/ARM/fp16-promote.ll
885–886 ↗(On Diff #38824)

Yes, we emit a mov here because the soft-float calling convention is different. I'll update the test to make this explicit.