azabaznov 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). ---------------- 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. 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
