This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Support enum and typedef args in TableGen BIFs
ClosedPublic

Authored by svenvh on Feb 4 2021, 8:47 AM.

Details

Summary

Add enum and typedef argument support to -fdeclare-opencl-builtins,
which was the last major missing feature.

Adding the remaining missing builtins is left as future work.

Diff Detail

Event Timeline

svenvh created this revision.Feb 4 2021, 8:47 AM
svenvh requested review of this revision.Feb 4 2021, 8:47 AM
Anastasia added inline comments.Feb 5 2021, 3:48 AM
clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
49

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.

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
640

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.

652

Same here, let's add an assert.

svenvh added inline comments.Feb 5 2021, 5:57 AM
clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
49

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.

clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp
640

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.

svenvh updated this revision to Diff 322123.Feb 8 2021, 8:37 AM

Do not assume cast to Enum/TypedefDecl is always successful.

The change looks good, could we test the diagnostic that is added?

svenvh updated this revision to Diff 323040.Feb 11 2021, 8:52 AM

Add test for new diagnostic.

Anastasia added inline comments.Feb 12 2021, 9:07 AM
clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
27

Actually, could we remove the typedef from the diagnostic because it exposes the implementation details unnecessarily? I believe the spec doesn't say what the type is aside from enums? As a matter of fact, we have some types implemented as typedefs that should have been a native types e.g. vector types that we might change one day.

svenvh added inline comments.Feb 12 2021, 9:21 AM
clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
27

I agree it's an implementation detail, but I can see reasons for keeping it: when there is an enum type with the same name available in the translation unit, then the message "type X not found" is not completely correct, because a type X exists, but it is of the wrong type. By prefixing "enum" or "typedef", the diagnostic is more accurate. This diagnostic is mainly targeted at Clang developers actually, because a user shouldn't see this diagnostic in any normal use case (with the driver setting both -fdeclare-opencl-builtins and -finclude-default-header).

Anastasia accepted this revision.Feb 16 2021, 4:47 AM

LGTM! Thanks!

clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
27

Ah, I see. Since the diagnostics are not going to be exposed during normal compilation it is less critical indeed... I guess the only issue is when we update the type implementation we might need more tests to change.

This revision is now accepted and ready to land.Feb 16 2021, 4:47 AM
This revision was automatically updated to reflect the committed changes.