Commit 9348da6d authored by Adam Procter's avatar Adam Procter Committed by Scott Cyphers

CPU backend support on macOS (#492)

* Allow caching of external dependencies (everything but TBB, which I can't figure out yet)

* Tweaks to get CPU backend building and linking (but not actually working) on macOS

* Tweaks to codegen/CPU backend to enable macOS (no builtin headers yet)

* Re-enable header caching stuff on macOS

* Suppress unsupported compiler warning for Apple Clang

* Fix includes for macOS compatibility
parent e57bb5c9
...@@ -26,20 +26,24 @@ endif() ...@@ -26,20 +26,24 @@ endif()
project (ngraph) project (ngraph)
SET( GCC_MIN_VERSION 4.8) SET(GCC_MIN_VERSION 4.8)
SET( CLANG_MIN_VERSION 3.8) SET(CLANG_MIN_VERSION 3.8)
SET(APPLE_CLANG_MIN_VERSION 9.0)
if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS GCC_MIN_VERSION) if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS GCC_MIN_VERSION)
message(FATAL_ERROR "GCC version must be at least ${GCC_MIN_VERSION}!") message(FATAL_ERROR "GCC version must be at least ${GCC_MIN_VERSION}!")
endif() endif()
elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang") elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
# require at least clang 3.8
if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS CLANG_MIN_VERSION) if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS CLANG_MIN_VERSION)
message(FATAL_ERROR "Clang version must be at least ${CLANG_MIN_VERSION}!") message(FATAL_ERROR "Clang version must be at least ${CLANG_MIN_VERSION}!")
endif() endif()
elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang")
if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS APPLE_CLANG_MIN_VERSION)
message(FATAL_ERROR "Apple Clang version must be at least ${APPLE_CLANG_MIN_VERSION}!")
endif()
else() else()
message(WARNING "You are using an unsupported compiler! Compilation has only been tested with Clang ( ${CLANG_MIN_VERSION} and up) and GCC ( ${GCC_MIN_VERSION} and up). ") message(WARNING "You are using an unsupported compiler. Compilation has only been tested with Clang (${CLANG_MIN_VERSION} and up), Apple Clang (${APPLE_CLANG_MIN_VERSION} and up), and GCC (${GCC_MIN_VERSION} and up).")
endif() endif()
if($ENV{NGRAPH_USE_PREBUILT_LLVM}) if($ENV{NGRAPH_USE_PREBUILT_LLVM})
......
...@@ -16,8 +16,7 @@ ...@@ -16,8 +16,7 @@
include(ExternalProject) include(ExternalProject)
if((NGRAPH_CPU_ENABLE OR NGRAPH_GPU_ENABLE) AND (NOT ${CMAKE_SYSTEM_NAME} MATCHES "Darwin") AND if((NGRAPH_CPU_ENABLE OR NGRAPH_GPU_ENABLE) AND (NOT ${CMAKE_SYSTEM_NAME} MATCHES "Windows"))
(NOT ${CMAKE_SYSTEM_NAME} MATCHES "Windows"))
set(CMAKE_DISABLE_SOURCE_CHANGES ON) set(CMAKE_DISABLE_SOURCE_CHANGES ON)
set(CMAKE_DISABLE_IN_SOURCE_BUILD ON) set(CMAKE_DISABLE_IN_SOURCE_BUILD ON)
...@@ -97,82 +96,163 @@ if((NGRAPH_CPU_ENABLE OR NGRAPH_GPU_ENABLE) AND (NOT ${CMAKE_SYSTEM_NAME} MATCHE ...@@ -97,82 +96,163 @@ if((NGRAPH_CPU_ENABLE OR NGRAPH_GPU_ENABLE) AND (NOT ${CMAKE_SYSTEM_NAME} MATCHE
set(LLVM_INCLUDE_DIR "${EXTERNAL_PROJECTS_ROOT}/llvm/include") # used by other external projects in current scope set(LLVM_INCLUDE_DIR "${EXTERNAL_PROJECTS_ROOT}/llvm/include") # used by other external projects in current scope
set(LLVM_LIB_DIR "${EXTERNAL_PROJECTS_ROOT}/llvm/lib" PARENT_SCOPE) set(LLVM_LIB_DIR "${EXTERNAL_PROJECTS_ROOT}/llvm/lib" PARENT_SCOPE)
set(LLVM_LINK_LIBS if(APPLE)
clangTooling set(LLVM_LINK_LIBS
clangFrontendTool clangTooling
clangFrontend clangFrontendTool
clangDriver clangFrontend
clangSerialization clangDriver
clangCodeGen clangSerialization
clangParse clangCodeGen
clangSema clangParse
clangStaticAnalyzerFrontend clangSema
clangStaticAnalyzerCheckers clangStaticAnalyzerFrontend
clangStaticAnalyzerCore clangStaticAnalyzerCheckers
clangAnalysis clangStaticAnalyzerCore
clangARCMigrate clangAnalysis
clangRewriteFrontend clangARCMigrate
clangEdit clangRewriteFrontend
clangAST clangEdit
clangLex clangAST
clangBasic clangLex
LLVMLTO clangBasic
LLVMPasses LLVMLTO
LLVMObjCARCOpts LLVMPasses
LLVMSymbolize LLVMObjCARCOpts
LLVMDebugInfoPDB LLVMSymbolize
LLVMDebugInfoDWARF LLVMDebugInfoPDB
LLVMMIRParser LLVMDebugInfoDWARF
LLVMCoverage LLVMMIRParser
LLVMTableGen LLVMCoverage
LLVMDlltoolDriver LLVMTableGen
LLVMOrcJIT LLVMDlltoolDriver
LLVMObjectYAML LLVMOrcJIT
LLVMLibDriver LLVMObjectYAML
LLVMOption LLVMLibDriver
LLVMX86Disassembler LLVMOption
LLVMX86AsmParser LLVMX86Disassembler
LLVMX86CodeGen LLVMX86AsmParser
LLVMGlobalISel LLVMX86CodeGen
LLVMSelectionDAG LLVMGlobalISel
LLVMAsmPrinter LLVMSelectionDAG
LLVMDebugInfoCodeView LLVMAsmPrinter
LLVMDebugInfoMSF LLVMDebugInfoCodeView
LLVMX86Desc LLVMDebugInfoMSF
LLVMMCDisassembler LLVMX86Desc
LLVMX86Info LLVMMCDisassembler
LLVMX86AsmPrinter LLVMX86Info
LLVMX86Utils LLVMX86AsmPrinter
LLVMMCJIT LLVMX86Utils
LLVMLineEditor LLVMMCJIT
LLVMInterpreter LLVMLineEditor
LLVMExecutionEngine LLVMInterpreter
LLVMRuntimeDyld LLVMExecutionEngine
LLVMCodeGen LLVMRuntimeDyld
LLVMTarget LLVMCodeGen
LLVMCoroutines LLVMTarget
LLVMipo LLVMCoroutines
LLVMInstrumentation LLVMipo
LLVMVectorize LLVMInstrumentation
LLVMScalarOpts LLVMVectorize
LLVMLinker LLVMScalarOpts
LLVMIRReader LLVMLinker
LLVMAsmParser LLVMIRReader
LLVMInstCombine LLVMAsmParser
LLVMTransformUtils LLVMInstCombine
LLVMBitWriter LLVMTransformUtils
LLVMAnalysis LLVMBitWriter
LLVMProfileData LLVMAnalysis
LLVMObject LLVMProfileData
LLVMMCParser LLVMObject
LLVMMC LLVMMCParser
LLVMBitReader LLVMMC
LLVMCore LLVMBitReader
LLVMBinaryFormat LLVMCore
LLVMSupport LLVMBinaryFormat
LLVMDemangle LLVMSupport
tinfo LLVMDemangle
z curses
m z
PARENT_SCOPE) m
PARENT_SCOPE)
else()
set(LLVM_LINK_LIBS
clangTooling
clangFrontendTool
clangFrontend
clangDriver
clangSerialization
clangCodeGen
clangParse
clangSema
clangStaticAnalyzerFrontend
clangStaticAnalyzerCheckers
clangStaticAnalyzerCore
clangAnalysis
clangARCMigrate
clangRewriteFrontend
clangEdit
clangAST
clangLex
clangBasic
LLVMLTO
LLVMPasses
LLVMObjCARCOpts
LLVMSymbolize
LLVMDebugInfoPDB
LLVMDebugInfoDWARF
LLVMMIRParser
LLVMCoverage
LLVMTableGen
LLVMDlltoolDriver
LLVMOrcJIT
LLVMObjectYAML
LLVMLibDriver
LLVMOption
LLVMX86Disassembler
LLVMX86AsmParser
LLVMX86CodeGen
LLVMGlobalISel
LLVMSelectionDAG
LLVMAsmPrinter
LLVMDebugInfoCodeView
LLVMDebugInfoMSF
LLVMX86Desc
LLVMMCDisassembler
LLVMX86Info
LLVMX86AsmPrinter
LLVMX86Utils
LLVMMCJIT
LLVMLineEditor
LLVMInterpreter
LLVMExecutionEngine
LLVMRuntimeDyld
LLVMCodeGen
LLVMTarget
LLVMCoroutines
LLVMipo
LLVMInstrumentation
LLVMVectorize
LLVMScalarOpts
LLVMLinker
LLVMIRReader
LLVMAsmParser
LLVMInstCombine
LLVMTransformUtils
LLVMBitWriter
LLVMAnalysis
LLVMProfileData
LLVMObject
LLVMMCParser
LLVMMC
LLVMBitReader
LLVMCore
LLVMBinaryFormat
LLVMSupport
LLVMDemangle
tinfo
z
m
PARENT_SCOPE)
endif()
endif() endif()
...@@ -20,7 +20,7 @@ include(ExternalProject) ...@@ -20,7 +20,7 @@ include(ExternalProject)
# Fetch and install MKL-DNN # Fetch and install MKL-DNN
#---------------------------------------------------------------------------------------------------------- #----------------------------------------------------------------------------------------------------------
if(NGRAPH_CPU_ENABLE AND NOT ${CMAKE_SYSTEM_NAME} MATCHES "Darwin") if(NGRAPH_CPU_ENABLE)
set(MKLDNN_GIT_REPO_URL https://github.com/intel/mkl-dnn) set(MKLDNN_GIT_REPO_URL https://github.com/intel/mkl-dnn)
set(MKLDNN_GIT_TAG "3e1f8f5") set(MKLDNN_GIT_TAG "3e1f8f5")
......
...@@ -22,13 +22,11 @@ if(NGRAPH_TBB_ENABLE) ...@@ -22,13 +22,11 @@ if(NGRAPH_TBB_ENABLE)
set(TBB_GIT_REPO_URL https://github.com/01org/tbb) set(TBB_GIT_REPO_URL https://github.com/01org/tbb)
set(TBB_GIT_TAG "tbb_2018") set(TBB_GIT_TAG "tbb_2018")
if(NOT ${CMAKE_SYSTEM_NAME} MATCHES "Darwin") configure_file(${CMAKE_SOURCE_DIR}/cmake/tbb_fetch.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/tbb/CMakeLists.txt)
configure_file(${CMAKE_SOURCE_DIR}/cmake/tbb_fetch.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/tbb/CMakeLists.txt) execute_process(COMMAND "${CMAKE_COMMAND}" -G "${CMAKE_GENERATOR}" .
execute_process(COMMAND "${CMAKE_COMMAND}" -G "${CMAKE_GENERATOR}" . WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/tbb")
WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/tbb") execute_process(COMMAND "${CMAKE_COMMAND}" --build .
execute_process(COMMAND "${CMAKE_COMMAND}" --build . WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/tbb")
WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/tbb")
set(TBB_ROOT ${CMAKE_CURRENT_BINARY_DIR}/tbb/tbb-src PARENT_SCOPE) set(TBB_ROOT ${CMAKE_CURRENT_BINARY_DIR}/tbb/tbb-src PARENT_SCOPE)
endif()
endif() endif()
...@@ -329,35 +329,33 @@ install(DIRECTORY ...@@ -329,35 +329,33 @@ install(DIRECTORY
FILES_MATCHING PATTERN "*.hpp" FILES_MATCHING PATTERN "*.hpp"
) )
if (NOT APPLE) install(DIRECTORY
install(DIRECTORY ${MKLDNN_LIB_DIR}/
${MKLDNN_LIB_DIR}/ DESTINATION "${NGRAPH_INSTALL_LIB}"
DESTINATION "${NGRAPH_INSTALL_LIB}" )
)
if (NGRAPH_TBB_ENABLE) if (NGRAPH_TBB_ENABLE)
install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/tbb_build/tbb_release/ install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/tbb_build/tbb_release/
DESTINATION ${NGRAPH_INSTALL_LIB} DESTINATION ${NGRAPH_INSTALL_LIB}
FILES_MATCHING PATTERN "libtbb.so.*" FILES_MATCHING PATTERN "libtbb.so.*"
) )
install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/tbb_build/tbb_debug/ install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/tbb_build/tbb_debug/
DESTINATION ${NGRAPH_INSTALL_LIB} DESTINATION ${NGRAPH_INSTALL_LIB}
FILES_MATCHING PATTERN "libtbb_debug.so.*" FILES_MATCHING PATTERN "libtbb_debug.so.*"
) )
endif() endif()
if (NGRAPH_ARGON_ENABLE) if (NGRAPH_ARGON_ENABLE)
install(DIRECTORY ${ARGON_TRANSFORMER_LIB_DIR}/ install(DIRECTORY ${ARGON_TRANSFORMER_LIB_DIR}/
DESTINATION ${NGRAPH_INSTALL_LIB} DESTINATION ${NGRAPH_INSTALL_LIB}
FILES_MATCHING PATTERN "*.so" FILES_MATCHING PATTERN "*.so"
) )
install(DIRECTORY ${ARGON_TRANSFORMER_INCLUDE_DIR}/ install(DIRECTORY ${ARGON_TRANSFORMER_INCLUDE_DIR}/
DESTINATION ${NGRAPH_INSTALL_INCLUDE} DESTINATION ${NGRAPH_INSTALL_INCLUDE}
FILES_MATCHING PATTERN "*.hpp" FILES_MATCHING PATTERN "*.hpp"
) )
install(DIRECTORY ${ARGON_TRANSFORMER_INCLUDE_DIR}/ install(DIRECTORY ${ARGON_TRANSFORMER_INCLUDE_DIR}/
DESTINATION ${NGRAPH_INSTALL_INCLUDE} DESTINATION ${NGRAPH_INSTALL_INCLUDE}
FILES_MATCHING PATTERN "*.h" FILES_MATCHING PATTERN "*.h"
) )
endif()
endif() endif()
...@@ -151,6 +151,8 @@ void codegen::StaticCompiler::initialize() ...@@ -151,6 +151,8 @@ void codegen::StaticCompiler::initialize()
// Prepare DiagnosticEngine // Prepare DiagnosticEngine
IntrusiveRefCntPtr<DiagnosticOptions> diag_options = new DiagnosticOptions(); IntrusiveRefCntPtr<DiagnosticOptions> diag_options = new DiagnosticOptions();
diag_options->ErrorLimit = 20; diag_options->ErrorLimit = 20;
diag_options->ShowCarets = false;
diag_options->ShowFixits = false;
IntrusiveRefCntPtr<DiagnosticIDs> diag_id(new DiagnosticIDs()); IntrusiveRefCntPtr<DiagnosticIDs> diag_id(new DiagnosticIDs());
DiagnosticsEngine diag_engine(diag_id, &*diag_options); DiagnosticsEngine diag_engine(diag_id, &*diag_options);
...@@ -351,6 +353,15 @@ void codegen::StaticCompiler::configure_search_path() ...@@ -351,6 +353,15 @@ void codegen::StaticCompiler::configure_search_path()
{ {
#ifdef USE_BUILTIN #ifdef USE_BUILTIN
load_headers_from_resource(); load_headers_from_resource();
#elif defined(__APPLE__)
add_header_search_path(EIGEN_HEADERS_PATH);
add_header_search_path(MKLDNN_HEADERS_PATH);
add_header_search_path(TBB_HEADERS_PATH);
add_header_search_path(NGRAPH_HEADERS_PATH);
add_header_search_path(INSTALLED_HEADERS_PATH);
add_header_search_path(CLANG_BUILTIN_HEADERS_PATH);
add_header_search_path("/Library/Developer/CommandLineTools/usr/include/c++/v1");
#else #else
// Add base toolchain-supplied header paths // Add base toolchain-supplied header paths
// Ideally one would use the Linux toolchain definition in clang/lib/Driver/ToolChains.h // Ideally one would use the Linux toolchain definition in clang/lib/Driver/ToolChains.h
......
...@@ -78,6 +78,13 @@ void codegen::ExecutionEngine::finalize() ...@@ -78,6 +78,13 @@ void codegen::ExecutionEngine::finalize()
void* codegen::ExecutionEngine::get_pointer_to_named_function(const std::string& func_name) void* codegen::ExecutionEngine::get_pointer_to_named_function(const std::string& func_name)
{ {
// For whatever reason, macOS seems to expect that we prefix this with an underscore.
#ifdef __APPLE__
std::string fname = "_" + func_name;
#else
const std::string& fname = func_name;
#endif
// set AbortOnFailure flag to false so call fails by returning nullptr // set AbortOnFailure flag to false so call fails by returning nullptr
return m_execution_engine->getPointerToNamedFunction(func_name, false); return m_execution_engine->getPointerToNamedFunction(fname, false);
} }
...@@ -423,10 +423,16 @@ void runtime::cpu::CPU_Emitter::EMITTER_DECL(EmitAbs) ...@@ -423,10 +423,16 @@ void runtime::cpu::CPU_Emitter::EMITTER_DECL(EmitAbs)
writer << emit_array1d(out[0]) << " =\n"; writer << emit_array1d(out[0]) << " =\n";
writer << "Eigen::abs(" << emit_array1d(args[0]) << ");\n"; writer << "Eigen::abs(" << emit_array1d(args[0]) << ");\n";
#else #else
// Some C++ implementations don't like it when we call std::abs on unsigned types, so we will
// avoid doing so here.
auto& result_element_type = out[0].get_element_type();
writer << "#pragma omp parallel for\n"; writer << "#pragma omp parallel for\n";
writer << "for (size_t i = 0; i < " << out[0].get_size() << "; i++)\n"; writer << "for (size_t i = 0; i < " << out[0].get_size() << "; i++)\n";
writer << "{\n"; writer << "{\n";
writer << " " << out[0].get_name() << "[i] = std::abs(" << args[0].get_name() << "[i]);\n"; writer << " " << out[0].get_name()
<< "[i] = " << (result_element_type.is_signed() ? "std::abs" : "") << "("
<< args[0].get_name() << "[i]);\n";
writer << "}\n"; writer << "}\n";
#endif #endif
writer.indent--; writer.indent--;
......
...@@ -831,7 +831,11 @@ using namespace ngraph::runtime; ...@@ -831,7 +831,11 @@ using namespace ngraph::runtime;
m_execution_engine->add_module(codegen_module); m_execution_engine->add_module(codegen_module);
m_execution_engine->finalize(); m_execution_engine->finalize();
m_compiled_function = m_execution_engine->find_function<EntryPoint_t>(function_name); m_compiled_function = m_execution_engine->find_function<EntryPoint_t>(function_name);
assert(m_compiled_function);
if (m_compiled_function == nullptr)
{
throw runtime_error("could not find compiled function");
}
m_is_compiled = true; m_is_compiled = true;
if (m_release_function) if (m_release_function)
......
...@@ -18,6 +18,7 @@ ...@@ -18,6 +18,7 @@
#include <cstdint> #include <cstdint>
#include <memory> #include <memory>
#include <string>
#include <vector> #include <vector>
#include <mkldnn.hpp> #include <mkldnn.hpp>
......
...@@ -14,6 +14,7 @@ ...@@ -14,6 +14,7 @@
* limitations under the License. * limitations under the License.
*******************************************************************************/ *******************************************************************************/
#include <string>
#include <typeindex> #include <typeindex>
#include <typeinfo> #include <typeinfo>
#include <unordered_set> #include <unordered_set>
......
...@@ -16,6 +16,7 @@ ...@@ -16,6 +16,7 @@
#pragma once #pragma once
#include <string>
#include <typeindex> #include <typeindex>
#include <typeinfo> #include <typeinfo>
#include <unordered_set> #include <unordered_set>
......
This diff is collapsed.
...@@ -16,6 +16,7 @@ ...@@ -16,6 +16,7 @@
#include <algorithm> #include <algorithm>
#include <memory> #include <memory>
#include <string>
#include <mkldnn.hpp> #include <mkldnn.hpp>
......
...@@ -14,7 +14,7 @@ ...@@ -14,7 +14,7 @@
# limitations under the License. # limitations under the License.
# ****************************************************************************** # ******************************************************************************
if (NGRAPH_CPU_ENABLE AND NOT APPLE) if (NGRAPH_CPU_ENABLE)
set (SRC set (SRC
main.cpp main.cpp
util.cpp util.cpp
......
...@@ -83,10 +83,21 @@ int main(int argc, char** argv) ...@@ -83,10 +83,21 @@ int main(int argc, char** argv)
} }
} }
vector<ResourceInfo> include_paths;
#ifdef __APPLE__
include_paths.push_back({EIGEN_HEADERS_PATH, {}, true});
include_paths.push_back({MKLDNN_HEADERS_PATH, {}, true});
#ifdef NGRAPH_TBB_ENABLE
include_paths.push_back({TBB_HEADERS_PATH, {}, true});
#endif
include_paths.push_back({NGRAPH_HEADERS_PATH, {}, true});
include_paths.push_back({CLANG_BUILTIN_HEADERS_PATH, {}, true});
include_paths.push_back({"/Library/Developer/CommandLineTools/usr/include/c++/v1", {}});
#else // __APPLE__
string cpp0 = find_path("/usr/include/x86_64-linux-gnu/c++/"); string cpp0 = find_path("/usr/include/x86_64-linux-gnu/c++/");
string cpp1 = find_path("/usr/include/c++/"); string cpp1 = find_path("/usr/include/c++/");
vector<ResourceInfo> include_paths;
include_paths.push_back({CLANG_BUILTIN_HEADERS_PATH, {}, true}); include_paths.push_back({CLANG_BUILTIN_HEADERS_PATH, {}, true});
include_paths.push_back({"/usr/include/x86_64-linux-gnu", {"asm", "sys", "bits", "gnu"}}); include_paths.push_back({"/usr/include/x86_64-linux-gnu", {"asm", "sys", "bits", "gnu"}});
include_paths.push_back({"/usr/include", {"asm", "sys", "bits", "gnu"}}); include_paths.push_back({"/usr/include", {"asm", "sys", "bits", "gnu"}});
...@@ -99,6 +110,7 @@ int main(int argc, char** argv) ...@@ -99,6 +110,7 @@ int main(int argc, char** argv)
include_paths.push_back({NGRAPH_HEADERS_PATH, {}, true}); include_paths.push_back({NGRAPH_HEADERS_PATH, {}, true});
#ifdef NGRAPH_TBB_ENABLE #ifdef NGRAPH_TBB_ENABLE
include_paths.push_back({TBB_HEADERS_PATH, {}, true}); include_paths.push_back({TBB_HEADERS_PATH, {}, true});
#endif
#endif #endif
if (output_path.empty()) if (output_path.empty())
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment