[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

Tue Apr 25 04:53:15 PDT 2023

arsenm added a comment.

In D142907#4288555 <https://reviews.llvm.org/D142907#4288555>, @efriedma wrote:

> If you have a library function that's built with "denormal-fp-math"="dynamic,dynamic", you can link it into code built in any mode, and LTO should be able to propagate that mode from the caller to the callee.  That doesn't require clang to do anything special; you can just specify -fdenormal-fp-math=dynamic while building the library, and the user specifies -fdenormal-fp-math=ieee while building their code.

That's essentially what this does. I think the part you are missing is the existing special treatment of the builtin device library functions. The default set of attributes for the current translation unit is forcibly set on functions in bitcode libraries linked in and internalized with -mlink-builtin-bitcode. We need to logically merge with the current translation unit's mode, or else we're potentially breaking the linked in function. The main reason I'm doing this in the first place is to move towards a model with less special treatment of these libraries.

> I guess you're worried specifically about ODR inline functions, defined in headers?  The user specifies a specific mode because they know their code honors it... but the user might not be aware of the effect on functions defined in library headers.  Other libraries in the same binary might use the same header, but specify a different mode.  So if the user specifies a denormal mode, we should ignore it for ODR inline functions, because they didn't actually mean to apply the denormal mode to those definitions?

No, the clang changes are to handle the headerless bitcode-only device libraries only. The user is supposed to be unaware the builtin libraries exist. They're an implementation detail managed by the clang driver (-mlink-builtin-bitcode is a cc1 only flag) and have a special contract with the compiler.

> I'm not sure about applying those semantics automatically; I don't think there's any precedent in clang for anything like this.  The closest thing I can think of is -fvisibility-inlines-hidden.  I'd prefer to RFC it separately from the rest of the patch, and loop in clang frontend owners, since the precedent we set here will apply to other sorts of attributes.

This isn't new, isn't end user facing, and isn't general purpose. This is the minimum required update to the existing -mlink-builtin-bitcode handling.

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

https://reviews.llvm.org/D142907