svenvh added inline comments.
================ Comment at: clang/include/clang/Basic/Builtins.def:88 // '__builtin_' prefix. It will be implemented in compiler-rt or libgcc. +// G -> this function uses generic address space (OpenCL). +// P -> this function uses pipes (OpenCL). ---------------- azabaznov wrote: > Anastasia wrote: > > azabaznov wrote: > > > Anastasia wrote: > > > > Anastasia wrote: > > > > > It might be better to avoid adding such limited language-specific > > > > > functionality into generic representation of Builtins. Do you think > > > > > could we could just introduce specific language modes, say: > > > > > > > > > > `OCLC_PIPES` > > > > > `OCLC_DSE` > > > > > `OCLC_GAS` > > > > > > > > > > and then check against those in `builtinIsSupported`? > > > > Btw another approach could be to do something similar to > > > > `TARGET_BUILTIN` i.e. list features in the last parameter as strings. > > > > We could add a separate macro for such builtins and just reuse target > > > > Builtins flow. This might be a bit more scalable in case we would need > > > > to add more of such builtins later on? > > > > > > > > It might be better to avoid adding such limited language-specific > > > > functionality into generic representation of Builtins. > > > > > > Nice idea! Though I think LanguageID is not designed to be used this way, > > > it's used only to check against specific language version. So it seems > > > pretty invasive. Also, function attributes seem more natural to me to > > > specify the type of the function. I don't know for sure which way is > > > better... > > > > > > > Btw another approach could be to do something similar to TARGET_BUILTIN > > > > i.e. list features in the last parameter as strings. > > > > > > I'd prefer to not use TARGET_BUILTIN as it operates on target feature, > > > but OpenCL feature is some other concept in clang... > > Buitlins handling is pretty vital for clang so if we extend common > > functionality for just a few built-in functions it might not justify the > > overhead in terms of complexity, parsing time or space... so we would need > > to dive in those aspects more before finalizing the design... if we can > > avoid it we should try... and I feel in this case there might be some good > > ways to avoid it. > > > > > Nice idea! Though I think LanguageID is not designed to be used this way, > > > it's used only to check against specific language version. So it seems > > > pretty invasive. Also, function attributes seem more natural to me to > > > specify the type of the function. I don't know for sure which way is > > > better... > > > > I think this `LanguageID` is only used for the purposes of Builtins, so > > there should be no issue in evolving it differently. With the documentation > > and adequate naming we can resolve the confusions in any. > > > > The whole idea of language options in clang is that it is not dependent on > > the target. But we have violated this design already. The whole concept of > > OpenCL 3.0 language features that are target-specific is misaligned with > > the original design in clang. > > > > > > > > Btw another approach could be to do something similar to > > > TARGET_BUILTIN i.e. list features in the last parameter as strings. > > > > > > I'd prefer to not use TARGET_BUILTIN as it operates on target feature, > > > but OpenCL feature is some other concept in clang... > > > > But we also have target features mirroring these, right? So I see no reason > > not to reuse what we already have... instead of adding another way to do > > the same or very similar thing... > > > > We could also consider extending the functionality slightly to use language > > features instead however I can't see the immediate benefit at this point... > > other than it might be useful in the future... but we can't know for sure. > > Buitlins handling is pretty vital for clang so if we extend common > > functionality for just a few built-in functions it might not justify the > > overhead in terms of complexity, parsing time or space... so we would need > > to dive in those aspects more before finalizing the design... if we can > > avoid it we should try... and I feel in this case there might be some good > > ways to avoid it. > > > >>Nice idea! Though I think LanguageID is not designed to be used this way, > >>it's used only to check against specific >?language version. So it seems > >>pretty invasive. Also, function attributes seem more natural to me to > >>specify the type of the function. I don't know for sure which way is > >>better... > > > > I think this LanguageID is only used for the purposes of Builtins, so there > > should be no issue in evolving it differently. With the documentation and > > adequate naming we can resolve the confusions in any. > > So yeah, I think reusing LanguageID is pretty doable and sounds like a good > idea. > > > > The whole idea of language options in clang is that it is not dependent on > > the target. But we have violated this design already. The whole concept of > > OpenCL 3.0 language features that are target-specific is misaligned with > > the original design in clang. > > > >>> Btw another approach could be to do something similar to TARGET_BUILTIN > >>> i.e. list features in the last parameter as strings. > >> I'd prefer to not use TARGET_BUILTIN as it operates on target feature, but > >> OpenCL feature is some other concept in clang... > > > > But we also have target features mirroring these, right? So I see no reason > > not to reuse what we already have... instead of adding another way to do > > the same or very similar thing... > > > > We could also consider extending the functionality slightly to use language > > features instead however I can't see the immediate benefit at this point... > > other than it might be useful in the future... but we can't know for sure. > > The whole problem is that features used by TARGET_BUILTIN are defined by > targets in llvm, for example avx512vl in X86. So making each target to > define, for example, //__opencl_c_device_enqueue// is not possible. The other > approach might be to outline the list of common subtarget features shared by > all targets, which is also not a good solution (despite the fact that there > are already some of those), but OpenCL features would fit into such concept. > > We discussed this a lot, but IMO OpenCL features are more about the language, > not about the target. For example, target might not support fp64, but > //__opencl_c_fp64// is supported so target can emulate fp64 (not a good > example, but I imagine this is possible). With this fact given, I thing > LANGBUILTIN fits better here then TARGET_BUILTIN. > > Nice idea! Though I think LanguageID is not designed to be used this way, > > it's used only to check against specific language version. So it seems > > pretty invasive. Also, function attributes seem more natural to me to > > specify the type of the function. I don't know for sure which way is > > better... > > I think this `LanguageID` is only used for the purposes of Builtins, so there > should be no issue in evolving it differently. With the documentation and > adequate naming we can resolve the confusions in any. `OCL_GAS` could be considered as a different language mode, namely it's the "OpenCL + Generic Addrspace" language mode. But it might be worth asking someone outside of the OpenCL community whether it's desirable to use the `LanguageID` enum in this way. ================ Comment at: clang/lib/Basic/Builtins.cpp:79 + bool OclBlocksUnsupported = + (LangOpts.getOpenCLCompatibleVersion() < 200 || !LangOpts.Blocks) && + (BuiltinInfo.Langs & OCL_BLOCKS); ---------------- azabaznov wrote: > This check is needed as //-cl-std=CL1.2// can be used together with > //-fblocks//. But in 3.0 block support requires //__opencl_c_device_enqueue// > feature. > This check is needed as -cl-std=CL1.2 can be used together with -fblocks. That surprised me, I cannot find anything in the OpenCL 1.2 spec about this. @Anastasia do you know if this is an (undocumented?) Clang extension? There are tests checking for this (e.g. `clang/test/Frontend/opencl.cl`), so we need this check to preserve the existing behavior indeed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118605/new/ https://reviews.llvm.org/D118605 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
