[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

Wed May 10 13:50:26 PDT 2023

zahiraam added a comment.

In D146148#4330971 <https://reviews.llvm.org/D146148#4330971>, @rjmccall wrote:

> In D146148#4330611 <https://reviews.llvm.org/D146148#4330611>, @zahiraam wrote:
>
>> In D146148#4221651 <https://reviews.llvm.org/D146148#4221651>, @rjmccall wrote:
>>
>>> In D146148#4220591 <https://reviews.llvm.org/D146148#4220591>, @zahiraam wrote:
>>>
>>>> In D146148#4220495 <https://reviews.llvm.org/D146148#4220495>, @aaron.ballman wrote:
>>>>
>>>>> In D146148#4220433 <https://reviews.llvm.org/D146148#4220433>, @zahiraam wrote:
>>>>>
>>>>>> In D146148#4220268 <https://reviews.llvm.org/D146148#4220268>, @rjmccall wrote:
>>>>>>
>>>>>>> Okay, so modifying `math.h` to use this attribute is acceptable?  That's great, that's definitely the best outcome for the compiler.
>>>>>>>
>>>>>>> Just a minor request about the diagnostic name, but otherwise LGTM.
>>>>>>
>>>>>> Yes. Added the attribute inside the included math.h header file.
>>>>>
>>>>> How does this work for, say, glibc, musl, or MSVC CRT? Won't those math.h headers lack the attribute and thus run into problems when used with Clang?
>>>>
>>>> Good point! @rjmccall are you thinking of something in particular with the attribute?
>>>
>>> Zahira, this is what I was asking you when I asked whether modifying the math.h header was acceptable: whether you were prepared to accept that the warning would only fire on system math.h headers that we'd modified, or whether you cared about making it work with non-cooperative headers.  I wasn't asking if you were willing to change the test code.
>>>
>>>> If not I guess we will have to rely on string comparison for all the typedef in the TU. Aaron pointed out the compile time overhead.
>>>
>>> Well, the compile-time overhead of doing this on every typedef *declaration* is way better than the overhead of doing this on every type lookup, at least.
>>>
>>> Anyway, there are three possibilities I can see:
>>>
>>> - We accept that this needs cooperative system headers.
>>> - We add a math.h compiler header that `#include_next`s the system math.h and then adds the attribute.  I believe you can just add an attribute to a typedef retroactively with something like `typedef float_t float_t __attribute__((whatever))`.
>>> - We do checks on every typedef declaration.  There's a builtin-identifier trick that we do with library functions that we should be able to generalize to typedefs, so you wouldn't need to actually do string comparisons, you'd just check whether the `IdentifierInfo*` was storing a special ID.  We'd set that up in `initializeBuiltins` at the start of the translation unit, so the overhead would just be that we'd create two extra `IdentifierInfo*`s in every TU.
>>>
>>> The builtin ID logic is currently specific to builtin *functions*, and I don't think we'd want to hack typedef names into that.  But the same storage in `IdentifierInfo*` is also used for identifying the ObjC context-sensitive keywords, by just offsetting the builtin IDs by the NUM_OBJC_KEYWORD.  You should be able to generalize that by also introducing a concept of a builtin *type* name, so that e.g. IDs in [0,NUM_OBJC_KEYWORDS) are ObjCKeywordKinds, IDs in [NUM_OBJC_KEYWORDS, NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES) are BuiltinTypeKinds (when you subtract NUM_OBJC_KEYWORDS), and IDs in [NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES,∞) are builtin function IDs (when you subtract NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES).
>>
>> Reading this more carefully... Does that mean that we initialize the float_t, double_t in initializeBuiltins  even if they are not used in the source code?
>
> Yes.  If we decide this is an overhead worth eliminating, we'll find a way to do it lazily to the builtins, and then we'll be able to take advantage of it here, too.  The builtins are a much larger contributor to overhead.
>
>> Also not sure how to define the NUM_BUILTIN_TYPES since I don't need to add it to TokenKinds.h? I was proposing to do something like this:
>> enum InterestingIdentifierKind {
>> #define Interesting_Identifier(X) X,
>> #include "clang/Basic/TokenKinds.def"
>>
>>   NUM_INTERESTING_IDENTIFIERS
>>
>> };
>> But I guess I don't need since we don't want to add additional storage. Do I understand things correctly?
>
> We should have an enum like this, yes.  And this is what we do in `IdentifierTable.h` for all the other kinds.  Alternatively, you can just hard-code the number and then static_assert that it's correct in some .cpp file.
>
> FWIW, I think I like NUM_INTERESTING_IDENTIFIERS as a name rather than NUM_BUILTIN_TYPES.

Okay, so I did add that in TokenKinds.h. Isn't that the right place for it? The same way it's done for the other builtins? And in TokenKinds.def I added the lines for the interesting identifiers?

In D146148#4330971 <https://reviews.llvm.org/D146148#4330971>, @rjmccall wrote:

> In D146148#4330611 <https://reviews.llvm.org/D146148#4330611>, @zahiraam wrote:
>
>> In D146148#4221651 <https://reviews.llvm.org/D146148#4221651>, @rjmccall wrote:
>>
>>> In D146148#4220591 <https://reviews.llvm.org/D146148#4220591>, @zahiraam wrote:
>>>
>>>> In D146148#4220495 <https://reviews.llvm.org/D146148#4220495>, @aaron.ballman wrote:
>>>>
>>>>> In D146148#4220433 <https://reviews.llvm.org/D146148#4220433>, @zahiraam wrote:
>>>>>
>>>>>> In D146148#4220268 <https://reviews.llvm.org/D146148#4220268>, @rjmccall wrote:
>>>>>>
>>>>>>> Okay, so modifying `math.h` to use this attribute is acceptable?  That's great, that's definitely the best outcome for the compiler.
>>>>>>>
>>>>>>> Just a minor request about the diagnostic name, but otherwise LGTM.
>>>>>>
>>>>>> Yes. Added the attribute inside the included math.h header file.
>>>>>
>>>>> How does this work for, say, glibc, musl, or MSVC CRT? Won't those math.h headers lack the attribute and thus run into problems when used with Clang?
>>>>
>>>> Good point! @rjmccall are you thinking of something in particular with the attribute?
>>>
>>> Zahira, this is what I was asking you when I asked whether modifying the math.h header was acceptable: whether you were prepared to accept that the warning would only fire on system math.h headers that we'd modified, or whether you cared about making it work with non-cooperative headers.  I wasn't asking if you were willing to change the test code.
>>>
>>>> If not I guess we will have to rely on string comparison for all the typedef in the TU. Aaron pointed out the compile time overhead.
>>>
>>> Well, the compile-time overhead of doing this on every typedef *declaration* is way better than the overhead of doing this on every type lookup, at least.
>>>
>>> Anyway, there are three possibilities I can see:
>>>
>>> - We accept that this needs cooperative system headers.
>>> - We add a math.h compiler header that `#include_next`s the system math.h and then adds the attribute.  I believe you can just add an attribute to a typedef retroactively with something like `typedef float_t float_t __attribute__((whatever))`.
>>> - We do checks on every typedef declaration.  There's a builtin-identifier trick that we do with library functions that we should be able to generalize to typedefs, so you wouldn't need to actually do string comparisons, you'd just check whether the `IdentifierInfo*` was storing a special ID.  We'd set that up in `initializeBuiltins` at the start of the translation unit, so the overhead would just be that we'd create two extra `IdentifierInfo*`s in every TU.
>>>
>>> The builtin ID logic is currently specific to builtin *functions*, and I don't think we'd want to hack typedef names into that.  But the same storage in `IdentifierInfo*` is also used for identifying the ObjC context-sensitive keywords, by just offsetting the builtin IDs by the NUM_OBJC_KEYWORD.  You should be able to generalize that by also introducing a concept of a builtin *type* name, so that e.g. IDs in [0,NUM_OBJC_KEYWORDS) are ObjCKeywordKinds, IDs in [NUM_OBJC_KEYWORDS, NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES) are BuiltinTypeKinds (when you subtract NUM_OBJC_KEYWORDS), and IDs in [NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES,∞) are builtin function IDs (when you subtract NUM_OBJC_KEYWORDS+NUM_BUILTIN_TYPES).
>>
>> Reading this more carefully... Does that mean that we initialize the float_t, double_t in initializeBuiltins  even if they are not used in the source code?
>
> Yes.  If we decide this is an overhead worth eliminating, we'll find a way to do it lazily to the builtins, and then we'll be able to take advantage of it here, too.  The builtins are a much larger contributor to overhead.
>
>> Also not sure how to define the NUM_BUILTIN_TYPES since I don't need to add it to TokenKinds.h? I was proposing to do something like this:
>> enum InterestingIdentifierKind {
>> #define Interesting_Identifier(X) X,
>> #include "clang/Basic/TokenKinds.def"
>>
>>   NUM_INTERESTING_IDENTIFIERS
>>
>> };
>> But I guess I don't need since we don't want to add additional storage. Do I understand things correctly?
>
> We should have an enum like this, yes.  And this is what we do in `IdentifierTable.h` for all the other kinds.  Alternatively, you can just hard-code the number and then static_assert that it's correct in some .cpp file.
>
> FWIW, I think I like NUM_INTERESTING_IDENTIFIERS as a name rather than NUM_BUILTIN_TYPES.

Okay but should this be added to TokenKinds.def or Builtin.defs? the patch is adding it to TokenKinds.

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

https://reviews.llvm.org/D146148