[PATCH] D126479: [Clang] Allow 'Complex float __attribute__((mode(HC)))'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/TargetInfo.h:885
   bool useObjCFPRetForRealType(FloatModeKind T) const {
+    assert(T <= FloatModeKind::Last);
     return RealTypeUsesObjCFPRet & (1 << (int)T);
----------------
This will assert if passed `NoFloat` -- is that intentional?

You should also add `&& "some helpful message"` to the assert predicate so that when the assert fails there's some more information about why the failure happened.


================
Comment at: clang/include/clang/Basic/TargetInfo.h:223-224
   unsigned HasAlignMac68kSupport : 1;
-  unsigned RealTypeUsesObjCFPRet : 3;
+  unsigned RealTypeUsesObjCFPRet : (1 << (int)FloatModeKind::Float) |
+                                   (1 << (int)FloatModeKind::Double);
   unsigned ComplexLongDoubleUsesFP2Ret : 1;
----------------
jolanta.jensen wrote:
> tahonermann wrote:
> > This doesn't look right to me. The size of the bit field would be `(1 << 1) | (1 << 2)` which is `0b110` which is 6, but more by coincidence than by construction. I think what we want is:
> >   unsigned RealTypeUsesObjCFPRet  : (int)FloatModeKind::Last + 1;
> Sorry. I mixed things up.
>I think what we want is:
>`unsigned RealTypeUsesObjCFPRet  : (int)FloatModeKind::Last + 1;`

I think this is wrong in two different ways. We need as many bits as required to store the enumerator value. The highest value is 255 (`NoFloat`), so we need at least 8 bits for that. But also, the last enumerator value is just that -- a value, not a width.

I'd probably skip the `Last` entry and force the bit-field to 8 bits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126479