[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: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:
> > aaron.ballman wrote:
> > > tahonermann wrote:
> > > > 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).
> > > Ahhhh, yeah, I definitely did not pick up the fact that these were used as part of a mask. I'm more used to masks being represented directly in the enumeration itself, e.g., `enum Whatever { FirstThing = 1 << 0, SecondThing = 1 << 1, ThirdThing = 1 << 2 };` These aren't really masks, they're shift amounts to be used to form a mask. Any reason we don't switch to that form to make it more clear (and simplify other code)? Actually, any reason why we don't want to switch the enumeration to use bitmask enums (https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/BitmaskEnum.h)?
> > I agree using the facilities provided by `BitmaskEnum.h` would be an improvement. Such a change request is arguably out of scope for this review, but if @jolanta.jensen wants to take that on, then great! I'm ok with the code as is (though a comment or some other explicit indicator that the enumeration and data member are used as a bit mask would be appreciated).
> I can happily make the change but I would prefer to carry it out in another patch as using facilities provided by BitmaskEnum is a non-functional change.
> I can happily make the change but I would prefer to carry it out in another patch as using facilities provided by BitmaskEnum is a non-functional change.

That's fine by me, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126479