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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to