This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Pass half or i16 types for NEON intrinsics
ClosedPublic

Authored by SjoerdMeijer on Mar 16 2018, 6:52 AM.

Details

Summary

For generating NEON intrinsics, this determines the NEON data type, and
whether it should be a half type or an i16 type. I.e., we always pass a half
type for AArch64, this hasn't changed, but now also for ARM but only when
FullFP16 is enabled, and i16 otherwise.

This is intended to be non-functional change, but together with the backend
work in https://reviews.llvm.org/D44538 which adds support for f16 vectors,
this enables adding the AArch32 FP16 (vector) intrinsics.

Diff Detail

Repository
rL LLVM

Event Timeline

SjoerdMeijer created this revision.Mar 16 2018, 6:52 AM
t.p.northover added inline comments.Mar 16 2018, 7:18 AM
include/clang/Basic/TargetInfo.h
365 ↗(On Diff #138692)

_Float16 doesn't seem to be supported anywhere in Clang (__fp16 is).

But we should probably clarify exactly what kind of support we mean here. This variable doesn't affect:

  • Whether __fp16 can be used at all in source.
  • Its ABI.

I'm actually slightly worried that when we document what it does affect it'll end up being an ARM implementation-detail.

lib/Basic/Targets/ARM.cpp
382 ↗(On Diff #138692)

This is now a bool rather than a bitfield, so false is definitely the best initializer (probably was before, too, but whatever).

lib/CodeGen/CGBuiltin.cpp
3456 ↗(On Diff #138692)

This check is equivalent to setting HasFloat16 in AArch64TargetInfo at the moment. Are you intending to change that (say by adding more uses of HasFloat16)?

Thanks for the review. Please see a first comment inline.

include/clang/Basic/TargetInfo.h
365 ↗(On Diff #138692)

I've added _Float16 support in Clang commit r312794: "Add _Float16 as a C/C++ source language type" :-)

t.p.northover added inline comments.Mar 16 2018, 7:39 AM
include/clang/Basic/TargetInfo.h
365 ↗(On Diff #138692)

Oh wow, that was ages ago! I have no idea how I still had a repo that old hanging around. Sorry about that.

Addressed comments: simplified the logic in GetNeonType.

t.p.northover added inline comments.Mar 16 2018, 9:10 AM
include/clang/Basic/TargetInfo.h
365 ↗(On Diff #138692)

I'm afraid I still think this description is iffy. For a start it is actually still affecting __fp16 rather than Float16 (since that's what float16_t is in the NEON header). And it's still too vague about just what "support" implies.

lib/CodeGen/CGBuiltin.cpp
3444 ↗(On Diff #138722)

Now that you set HasFloat16 in AArch64, I believe Arch is unused.

SjoerdMeijer retitled this revision from [ARM] Add HasFloat16 to TargetInfo to [ARM] Pass half or i16 types for NEON intrinsics.

Removed unused function argument, and renamed HasFloat16

t.p.northover accepted this revision.Mar 19 2018, 3:52 AM

Thanks. I think this looks reasonable now.

This revision is now accepted and ready to land.Mar 19 2018, 3:52 AM

Thanks a lot for your help and reviews.

This revision was automatically updated to reflect the committed changes.