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