[PATCH] D78979: OpenCL: Include builtin header by default

Tue Apr 28 04:47:19 PDT 2020

svenvh added a comment.

In D78979#2007356 <https://reviews.llvm.org/D78979#2007356>, @Anastasia wrote:

> Originally we didn't want to include the header by default for 2 reasons:
>
> 1. Many vendors used their own header.
> 2. It takes long time to parse the header.
>
>   However, we might be in a different position now because the header exists for long time and I think we have fixed all the issues so it should be recommended to use it instead of vendor's header. It is generally good to include it by default for the upstream user. However, I am still concerned with the parsing time. To mitigate this we have developed an alternative way to include the functions utilizing TableGen: https://llvm.org/devmtg/2019-10/talk-abstracts.html#lit5 Unfortunately, we are not that far with testing yet to be able to use it by default. But hopefully we should be able to switch to is as a default option at some point soon.

The main thing that's missing from the TableGen approach is support for builtins taking enum arguments, and then there is completeness testing as you mentioned.  So I think it is not ready yet to become the default way of handling OpenCL builtins.

> To summarize I am generally not against the inclusion of the header by default and I think it's best if we just reuse the standard non-OpenCL specific flags.

I agree.

> We would just need to understand how it will work with the fast TableGen-based solution that is enabled using `-fdeclare-opencl-builtins`.

The combination of `-fdeclare-opencl-builtins` and `finclude-default-header` currently causes a smaller include file (that is fast to parse) to be included. So we should include the small header by default for `-fdeclare-opencl-builtins`.

> I am looping in @svenvh to discuss this further. Sven, do you think we should rename this flag due to current proposed change? Should it be moved out of `-cc1` too?

I would leave `-fdeclare-opencl-builtins` as it is, until it is complete and taken out of its "experimental" status.  Then we can discuss if we want to use TableGen instead of the header as a default.

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

https://reviews.llvm.org/D78979