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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
