[PATCH] D44561: [ARM] Add HasFloat16 to TargetInfo

[Tim Northover via Phabricator via cfe-commits]

t.p.northover added inline comments.


================
Comment at: include/clang/Basic/TargetInfo.h:365
 
+  /// \brief Determine whether _Float16 is supported on this target.
+  virtual bool hasFloat16Type() const { return HasFloat16; }
----------------
t.p.northover wrote:
> SjoerdMeijer wrote:
> > t.p.northover wrote:
> > > `_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.
> > > 
> > > 
> > I've added _Float16 support in Clang commit r312794: "Add _Float16 as a C/C++ source language type" :-)
> Oh wow, that was ages ago! I have no idea how I still had a repo that old hanging around. Sorry about that.
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.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:3444
                                      NeonTypeFlags TypeFlags,
                                      llvm::Triple::ArchType Arch,
+                                     bool HasFloat16=true,
----------------
Now that you set `HasFloat16` in AArch64, I believe `Arch` is unused.


https://reviews.llvm.org/D44561