Anastasia added inline comments.
================ Comment at: include/clang/Basic/OpenCLExtensionTypes.def:27 + +INTEL_SGAVC_TYPE(mce_payload_t, McePayload) +INTEL_SGAVC_TYPE(ime_payload_t, ImePayload) ---------------- AlexeySotkin wrote: > Anastasia wrote: > > AlexeySachkov wrote: > > > Anastasia wrote: > > > > From the specification of this extension I can't quite see if these > > > > types have to be in Clang instead of the header. Can you please > > > > elaborate on any example where it wouldn't be possible for this type to > > > > be declared in the header using the technique explained in: > > > > https://clang.llvm.org/docs/UsersManual.html#opencl-extensions > > > We cannot define these types in header because their layout is not > > > defined in specification, i.e. all of these types are opaque > > This is not the reason to add functionality to Clang. You can easily sort > > such things with target specific headers or even general headers (see > > `ndrange_t` for example). Spec doesn't have to describe everything. The > > question is whether there is something about those types that can't be > > handled using standard include mechanisms. Usually it's prohibited > > behaviors that can't be represented with such mechanisms. Like if some > > operations have to be disallowed or allowed (since in OpenCL C you can't > > define user defined operators) with the types. > > > > I think the trend is to avoid adding complexity into Clang, unless there is > > no other way to implement some feature correctly. > Major part of these types must support initialization only by zero. > intel_sub_group_avc_mce_payload_t and intel_sub_group_avc_mce_result_t must > support initialization only via special builtins defined in the spec. > Corresponding errors must be reported. I think we can't implement this > behavior using standard include mechanism, can we? > > Possible value of the additional complexity, except builtin declaration, is > that the patch is quite generic. So next time anyone wants to implement an > extension with a type restrictions which can't be handled with the include > mechanism, all that they needs to do is to modify this single file. > Major part of these types must support initialization only by zero. > intel_sub_group_avc_mce_payload_t and intel_sub_group_avc_mce_result_t must > support initialization only via special builtins defined in the spec. > Corresponding errors must be reported. I think we can't implement this > behavior using standard include mechanism, can we? Are these restrictions not mentioned in the specification document then? Or is it not the final version yet (not sure since it says First Draft). Do you plan to add the diagnostics for the restrictions afterwards? It doesn't have to be in the same patch, but just checking because if not I don't think it would make sense to go this route. > Possible value of the additional complexity, except builtin declaration, is > that the patch is quite generic. So next time anyone wants to implement an > extension with a type restrictions which can't be handled with the include > mechanism, all that they needs to do is to modify this single file. > It seems reasonable to implement this extra mechanism, provided that there are more of similar use cases. Btw, considering that there are some modifications to core language spec restriction sections in this document, does this extension invalidate or change any core language rules that might affect parsing/diagnostics? Repository: rC Clang https://reviews.llvm.org/D51484 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
