[PATCH] D96051: [OpenCL] Support enum and typedef args in TableGen BIFs

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added inline comments.


================
Comment at: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl:38
+typedef uint cl_mem_fence_flags;
+#define CLK_GLOBAL_MEM_FENCE   0x02
+
----------------
Anastasia wrote:
> Should we add this conditionally if the base header is not included?
> 
> In the subsequent patches where you will add other functions, we should make sure that the base header indeed contains the declarations we use in tablegen.
> Should we add this conditionally if the base header is not included?

It is already inside an `#ifdef NO_HEADER`, see line 21.

> In the subsequent patches where you will add other functions, we should make sure that the base header indeed contains the declarations we use in tablegen.

Not sure what you mean...  When we test a builtin function that relies on an enum or typedef from the base header, Sema won't succeed if the enum or typedef isn't available.


================
Comment at: clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:727
+  }
+  return S.Context.getEnumType(Result.getAsSingle<EnumDecl>());
+}
----------------
Anastasia wrote:
> I think we should add an assert that Result.getAsSingle<EnumDecl>() indeed holds. Consider if instead of using base header the types are defined manually and they are regular integers, not enums.
That is a good point.  I think it would be even better to handle such a case properly with a diagnostic, and not rely on just an assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96051