[PATCH] D57188: Disable _Float16 for non ARM/SPIR Targets

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 5 inline comments as done.
erichkeane added inline comments.


================
Comment at: include/clang/Basic/TargetInfo.h:66
   bool HasFloat128;
+  bool HasFloat16;
   unsigned char PointerWidth, PointerAlign;
----------------
SjoerdMeijer wrote:
> I think this is the same as `HasLegalHalfType`, and we can (re)use that.  Or, at least, don't think we need both `HasLegalHalfType` and `HasFloat16`. For context, I needed `HasLegalHalfType` for argument passing, but it looks like it can serve another purpose now.
> 
> Out of curiousity, I was wondering if specifying:
> 
>   KEYWORD(_Float16                    , HALFSUPPORT)
> 
> in TokenKids.def is an alternative approach (it is currently set to KEYALL). Thus, enable the keyword when `LangOpts.Half` is set. By adding this `HasFloat16` property here in clang's targetinfo, we're sort of defining again how targets support different types. I.e., if you throw a `half` type at the backend, the TypeLegalizer will deal with it in one way or another. Perhaps disabling `_Float16` can be achieved by disabling the keyword. But I do see that the big advantage of this patch is the much nicer error message (otherwise we would get something like  "unknown type name '_Float16'").
> 
LangOpts.Half (and thus HALFSUPPORT) seems to be specific to OpenCL.  It seems to me that the two ARE different, the Half type and the _Float16 type are different semantically and from different standards, so I think that isn't sufficient.  

Additionally, from a diagnostics perspective I think it is worse to do it in the TokenKinds.def. If you do it in TokenKinds.def, you get "Type Expected" or "Unknown Type Name", which could encourage someone to write their own.  The error message in this patch (Not supported on this target) strongly states that it is a reserved word and is not to be defined.  This is particularly important for the targets that intend to add support for this in the future.

Additionally, HasLegalHalfType has slightly different semantics in the ARM case.  ARM seems to still want _Float16 to work even without the +fullfp16 support.  It seems to be OK (based on CodeGen and the tests written to validate this) with allowing the _Float16 support to work with different code generation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57188/new/

https://reviews.llvm.org/D57188