This is an archive of the discontinued LLVM Phabricator instance.

Moving OpenCLBuiltins.td and remove unused include
ClosedPublic

Authored by Pierre on Jun 4 2019, 3:33 AM.

Details

Summary

This patch makes the changes requested by thakis in the patch https://reviews.llvm.org/D60763 :

• thakis.
From what I can tell, the only client of OpenCLBuiltins.td is currently lib/Sema. Do you expect that to change? If not, does it make more sense to move the .td file to there?
Do you expect OpenCLBuiltins.td to include other files in the future? At the moment, the -I ${CMAKE_CURRENT_SOURCE_DIR}/../../ bit in the cmake file isn't needed for anything.

Diff Detail

Repository
rL LLVM

Event Timeline

Pierre created this revision.Jun 4 2019, 3:33 AM
Pierre removed a subscriber: thakis.
thakis accepted this revision.Jun 4 2019, 2:40 PM

Thanks!

This revision is now accepted and ready to land.Jun 4 2019, 2:40 PM
thakis added a comment.Jun 4 2019, 2:41 PM

Actually, maybe the file could even be in lib/Sema instead of in include/clang/Sema if it's private to Sema?

Pierre updated this revision to Diff 203101.Jun 5 2019, 1:29 AM

At the right location this time

Pierre updated this revision to Diff 203795.Jun 10 2019, 3:57 AM

It was not necessary to add the .inc file in the add_clang_library

svenvh added inline comments.Jun 11 2019, 7:12 AM
clang/lib/Sema/CMakeLists.txt
75 ↗(On Diff #203795)

This makes clangSema dependent on the tablegen invocation, but actually SemaLookup.cpp which includes the generated .inc file should be made dependent on ClangOpenCLBuiltinsImpl to ensure tablegen runs before compiling SemaLookup.cpp.

Pierre updated this revision to Diff 204061.Jun 11 2019, 7:19 AM

Dependencies were not satisfied with the last version:
clangSema was built after ClangOpenCLBuiltinsImpl,
but it didn't imply that OpenCLBuiltins.inc had to be built before SemaLookup.cpp

svenvh accepted this revision.Jun 13 2019, 2:43 AM

LGTM.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 2:52 AM

Getting build regression on Linux (Fedora 29 x86_64):

make[2]: *** No rule to make target 'ClangOpenCLBuiltinsImpl', needed by 'tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaLookup.cpp.o'.  Stop.

Using command:

cmake ../llvm-monorepo/llvm/ -DCMAKE_BUILD_TYPE=Release  -DLLVM_USE_LINKER=gold -DLLVM_ENABLE_PROJECTS="lldb;clang;lld"  -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_ENABLE_ASSERTIONS=ON

Sorry for the breakage and thanks for reporting!

Reverted in r363376 for now, we'll look into it.

Recommitted in r363541.