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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann 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);
----------------
aaron.ballman wrote:
> 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.
Yes, this should assert if `NoFloat` is passed.

I agree adding a message would be helpful.


================
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;
----------------
aaron.ballman wrote:
> 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.
`RealTypeUsesObjCFPRet` is used as a bit mask indexed by the `FloatModeKind` enumerator values; the `X86_32TargetInfo` constructor in `clang/lib/Basic/Targets/X86.h` has the following code:

  420     // Use fpret for all types.
  421     RealTypeUsesObjCFPRet =
  422         ((1 << (int)FloatModeKind::Float) | (1 << (int)FloatModeKind::Double) |
  423          (1 << (int)FloatModeKind::LongDouble));

`NoFloat` is a special case for which a mask bit is not needed.

I think this code is correct as is, but Aaron's comment suggests some clarification would be useful. Perhaps the data member should be renamed to something like `RealTypeUsesObjCFPRetMask` (though I suspect a better name can be found).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126479