tahonermann 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;
----------------
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).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126479/new/
https://reviews.llvm.org/D126479
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
