This is an archive of the discontinued LLVM Phabricator instance.

Differential D72141

[mlir][spirv] Fix ADDITIONAL_HEADER_DIRS for SPIR-V libraries
ClosedPublic

Authored by antiagainst on Jan 3 2020, 5:04 AM.

Details

Reviewers
mravishankar
denis13
rriddle
nicolasvasilache
Commits
rG1570084d3343: [mlir][spirv] Fix ADDITIONAL_HEADER_DIRS for SPIR-V libraries
Summary

SPIRV/ headers live under mlir/Dialect.

Diff Detail

Event Timeline

antiagainst created this revision.Jan 3 2020, 5:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2020, 5:04 AM
Herald added subscribers: llvm-commits, lucyrfox, mgester and 8 others. · View Herald Transcript
merge_guards_bot added a subscriber: merge_guards_bot.Jan 3 2020, 5:15 AM

Unit tests: pass. 61175 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Harbormaster failed remote builds in B43221: Diff 236026!Jan 3 2020, 5:19 AM
rriddle accepted this revision.Jan 3 2020, 9:49 AM
This revision is now accepted and ready to land.Jan 3 2020, 9:49 AM
Closed by commit rG1570084d3343: [mlir][spirv] Fix ADDITIONAL_HEADER_DIRS for SPIR-V libraries (authored by antiagainst). · Explain WhyJan 3 2020, 12:11 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Jan 16 2020, 10:41 PM
mlir/lib/Dialect/SPIRV/Transforms/CMakeLists.txt
6

Why is this needed?

The build is passing for me without this. I can't find where a header would be included from one of these source files?

Herald added a reviewer: nicolasvasilache. · View Herald TranscriptJan 16 2020, 10:41 PM
Herald added subscribers: liufengdb, aartbik. · View Herald Transcript
antiagainst marked an inline comment as done.Jan 17 2020, 4:57 AM
antiagainst added inline comments.
mlir/lib/Dialect/SPIRV/Transforms/CMakeLists.txt
6

This is not needed for build. It is for aiding IDEs if I understand the original intent of ADDTIONAL_HEADERS_DIRS correctly (https://reviews.llvm.org/D7460). This is also to be consistent with other CMakeLists.txt we have in MLIR.

Revision Contents

PathSize
mlir/
lib/
Dialect/
SPIRV/
2 lines
Serialization/
2 lines
Transforms/
3 lines

Diff 236104

mlir/lib/Dialect/SPIRV/CMakeLists.txt

set(LLVM_TARGET_DEFINITIONS SPIRVCanonicalization.td) set(LLVM_TARGET_DEFINITIONS SPIRVCanonicalization.td)
mlir_tablegen(SPIRVCanonicalization.inc -gen-rewriters) mlir_tablegen(SPIRVCanonicalization.inc -gen-rewriters)
add_public_tablegen_target(MLIRSPIRVCanonicalizationIncGen) add_public_tablegen_target(MLIRSPIRVCanonicalizationIncGen)
add_llvm_library(MLIRSPIRV add_llvm_library(MLIRSPIRV
DialectRegistration.cpp DialectRegistration.cpp
LayoutUtils.cpp LayoutUtils.cpp
SPIRVDialect.cpp SPIRVDialect.cpp
SPIRVOps.cpp SPIRVOps.cpp
SPIRVLowering.cpp SPIRVLowering.cpp
SPIRVTypes.cpp SPIRVTypes.cpp
TargetAndABI.cpp TargetAndABI.cpp
ADDITIONAL_HEADER_DIRS ADDITIONAL_HEADER_DIRS
${MLIR_MAIN_INCLUDE_DIR}/mlir/SPIRV ${MLIR_MAIN_INCLUDE_DIR}/mlir/Dialect/SPIRV
) )
add_dependencies(MLIRSPIRV add_dependencies(MLIRSPIRV
MLIRSPIRVAvailabilityIncGen MLIRSPIRVAvailabilityIncGen
MLIRSPIRVCanonicalizationIncGen MLIRSPIRVCanonicalizationIncGen
MLIRSPIRVEnumAvailabilityIncGen MLIRSPIRVEnumAvailabilityIncGen
MLIRSPIRVEnumsIncGen MLIRSPIRVEnumsIncGen
MLIRSPIRVOpsIncGen MLIRSPIRVOpsIncGen
Show All 11 Lines

mlir/lib/Dialect/SPIRV/Serialization/CMakeLists.txt

add_llvm_library(MLIRSPIRVSerialization add_llvm_library(MLIRSPIRVSerialization
Deserializer.cpp Deserializer.cpp
Serializer.cpp Serializer.cpp
SPIRVBinaryUtils.cpp SPIRVBinaryUtils.cpp
TranslateRegistration.cpp TranslateRegistration.cpp
ADDITIONAL_HEADER_DIRS ADDITIONAL_HEADER_DIRS
${MLIR_MAIN_INCLUDE_DIR}/mlir/SPIRV ${MLIR_MAIN_INCLUDE_DIR}/mlir/Dialect/SPIRV
) )
add_dependencies(MLIRSPIRVSerialization add_dependencies(MLIRSPIRVSerialization
MLIRSPIRVSerializationGen) MLIRSPIRVSerializationGen)
target_link_libraries(MLIRSPIRVSerialization target_link_libraries(MLIRSPIRVSerialization
MLIRIR MLIRIR
MLIRSPIRV MLIRSPIRV
MLIRSupport) MLIRSupport)

mlir/lib/Dialect/SPIRV/Transforms/CMakeLists.txt

add_llvm_library(MLIRSPIRVTransforms add_llvm_library(MLIRSPIRVTransforms
DecorateSPIRVCompositeTypeLayoutPass.cpp DecorateSPIRVCompositeTypeLayoutPass.cpp
LowerABIAttributesPass.cpp LowerABIAttributesPass.cpp
ADDITIONAL_HEADER_DIRS
${MLIR_MAIN_INCLUDE_DIR}/mlir/Dialect/SPIRV
mehdi_aminiUnsubmitted
Not Done
ReplyInline Actions

Why is this needed?

The build is passing for me without this. I can't find where a header would be included from one of these source files?

mehdi_amini: Why is this needed? The build is passing for me without this. I can't find where a header…
antiagainstAuthorUnsubmitted
Done
ReplyInline Actions

This is not needed for build. It is for aiding IDEs if I understand the original intent of ADDTIONAL_HEADERS_DIRS correctly (https://reviews.llvm.org/D7460). This is also to be consistent with other CMakeLists.txt we have in MLIR.

antiagainst: This is not needed for build. It is for aiding IDEs if I understand the original intent of…
) )
target_link_libraries(MLIRSPIRVTransforms target_link_libraries(MLIRSPIRVTransforms
MLIRPass MLIRPass
MLIRSPIRV MLIRSPIRV
) )