[PATCH] D65456: [OpenCL] Add generic type handling for builtin functions

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh marked 23 inline comments as done.
svenvh added a subscriber: Pierre.
svenvh added inline comments.


================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:51
-// Helper class to store type access qualifiers (volatile, const, ...).
-class Qualifier<string _QualName> {
-  string QualName = _QualName;
----------------
Anastasia wrote:
> Are the qualifiers added elsewhere?
Good point, this change should be part of the next patch (D63442).  I've taken it out of this patch.


================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:221
+
+// GenType definitions.
+def FGenTypeN   : GenericType<"FGenTypeN", TLFloat, VecAndScalar>;
----------------
Anastasia wrote:
> Is this float GenType?
We will be adding more definitions for integers here in followup patches.


================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:222
+// GenType definitions.
+def FGenTypeN   : GenericType<"FGenTypeN", TLFloat, VecAndScalar>;
+// Generate basic GenTypes.  Names are like: GenTypeFloatVecAndScalar.
----------------
Anastasia wrote:
> Would it make sense to use the same name scheme as below?
No, because FGenType is for all floating point types (half / float / double), as it uses the `TLFloat` type list.  `GenTypeFloatVecAndScalar` is for the float type (i.e. fp32) only.

I will update the comments of both definitions to clarify the difference.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:680
+/// \param OpenCLBuiltin (in) The signature currently handled.
+/// \param GenTypeMaxCnt (out) Maximum number of types contained in a generic
+///        type used as return type or as argument.
----------------
Anastasia wrote:
> What if both return and args use GenType, does `GenTypeMaxCnt` return max of all? Or do they have to align?
They have to be compatible, otherwise we will hit the `llvm_reachable` below.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:708
+  }
+  for (unsigned Index = 0; Index < ArgTypes.size(); Index++) {
+    unsigned TypeCntAtIndex = ArgTypes[Index].size();
----------------
Anastasia wrote:
> I don't get this logic?
While trying to clarify this, I realized this checking should probably be moved to the TableGen emitter, as this is checking validity of the .td input so ideally we should check that at compiler compile time.  @Pierre mentioned that this might not be trivial, but I'll have a look at it.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:70
 namespace {
 class BuiltinNameEmitter {
 public:
----------------
Anastasia wrote:
> Perhaps not something for this patch, but I am wondering if a better name for this class would be `OpenCLBuiltinTrieEmitter`?
It's emitting more than a trie; also tables and structs.  How about `OpenCLBuiltinEmitter` (which aligns nicely with the file name)?


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:94
+  // \param List (out) List to fill with the extracted types.
+  void ExtractEnumTypes(std::vector<Record *> &Types,
+                        StringMap<bool> &TypesSeen, std::string &Output,
----------------
Anastasia wrote:
> I am a bit confused about the purpose of this function as input and output seem to be both vectors of the Record. It might make sense to explain the difference. :)
Clarified comment.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:225
+
+  // Generic types are defined at the end of the enum.
+  std::vector<Record *> GenTypes =
----------------
Anastasia wrote:
> I find this comment confusing... may be because it belongs to the code later. I am not sure it adds any value.
Elaborated.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:281
     auto it =
-        std::find_if(SignatureSet.begin(), SignatureSet.end(),
+        std::find_if(SignaturesList.begin(), SignaturesList.end(),
                      [&](const std::pair<std::vector<Record *>, unsigned> &a) {
----------------
Anastasia wrote:
> Is this logic to avoid duplicate signatures? If so it might be worth explaining it.
Added comment.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:377
+// is returned.
+static void OCL2Qual(ASTContext &Context, const OpenCLTypeStruct &Ty,
+                     std::vector<QualType> &QT) {
----------------
Anastasia wrote:
> I feel it would be good to explain the structure of the function we are trying to generate i.e something like:
> - We first generate switch/case statement over all type names that populates the list of type IDs
> - Then we walk over the type IDs from the list and map them to the AST type and attributes accordingly.
Added comment.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:404
+    // the plain scalar types for now; the ExtVector type will be added after
+    // the switch statement such that it is only added for the case matching
+    // the input to the OCL2Qual function.
----------------
Anastasia wrote:
> I am not sure what you are trying to say about ExtVector...
Rephrased.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:457
+  // Add ExtVector types if this was a generic type, as the switch statement
+  // above only populated the list with scalar types.  This completes the
+  // construction of the Cartesian product of (vector sizes) x (types).
----------------
Anastasia wrote:
> I am really confused as the above comment on line 432 makes examples of non scalar types too i.e. `int4`.
I guess you got confused about the two different steps; I have rephrased the comment on line 432.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65456