Skip to content

Conversation

@georgthegreat
Copy link
Contributor

At the time the code can not be compiled by NVCC.

@georgthegreat
Copy link
Contributor Author

@RyanUnderhill, it looks like this PR got lost.

@baijumeswani
Copy link
Contributor

I am able to build ort without any error for cuda ep.

Would you share the build command and error you're seeing?

@RyanUnderhill
Copy link
Contributor

@RyanUnderhill, it looks like this PR got lost.

Yep, can you say what compiler version you're using? My guess is GCC based on a quickly online search.

I don't see any unicode strings in the file, would be amusing to know how this was added. VC++ only does it if you type unicode into the file, so it might have happened accidentally.

@georgthegreat
Copy link
Contributor Author

This is the failing command and the begginning of the error list:

/home/thegeorg/.ya/tools/v4/2410761119/bin/nvcc --expt-extended-lambda --expt-relaxed-constexpr --compiler-bindir=/home/thegeorg/.ya/tools/v4/1886578148/bin/clang -I/home/thegeorg/.ya/tools/v4/1966560555/usr/include/x86_64-linux-gnu -w -c /home/thegeorg/arcadia/contrib/libs/onnx_runtime/onnxruntime/contrib_ops/cuda/math/fft_ops_impl.cu -o /home/thegeorg/.ya/build/build_root/mykz/000195/contrib/libs/onnx_runtime/onnxruntime/core/providers/cuda/__/__/__/contrib_ops/cuda/math/fft_ops_impl.cu.o -I/home/thegeorg/.ya/build/build_root/mykz/000195 -I/home/thegeorg/arcadia -I/home/thegeorg/arcadia/contrib/libs/eigen -I/home/thegeorg/arcadia/contrib/libs/nsync/public -I/home/thegeorg/arcadia/contrib/libs/nvidia/cudnn -I/home/thegeorg/arcadia/contrib/libs/onnx_runtime -I/home/thegeorg/arcadia/contrib/libs/onnx_runtime/_deps/gsl-src/include -I/home/thegeorg/arcadia/contrib/libs/onnx_runtime/_deps/safeint-src -I/home/thegeorg/arcadia/contrib/libs/onnx_runtime/include/onnxruntime -I/home/thegeorg/arcadia/contrib/libs/onnx_runtime/include/onnxruntime/core/session -I/home/thegeorg/arcadia/contrib/libs/onnx_runtime/onnxruntime -I/home/thegeorg/arcadia/contrib/libs/linux-headers -I/home/thegeorg/arcadia/contrib/libs/linux-headers/_nf -I/home/thegeorg/arcadia/contrib/libs/cxxsupp/libcxx/include -I/home/thegeorg/arcadia/contrib/libs/cxxsupp/libcxxrt/include -I/home/thegeorg/arcadia/contrib/libs/nvidia/thrust -I/home/thegeorg/arcadia/contrib/libs/nvidia/cub -I/home/thegeorg/arcadia/contrib/libs/nsync/public -I/home/thegeorg/arcadia/contrib/libs/zlib/include -I/home/thegeorg/arcadia/contrib/libs/double-conversion -I/home/thegeorg/arcadia/contrib/libs/libc_compat/include/readpassphrase -I/home/thegeorg/arcadia/contrib/libs/tbb/include -I/home/thegeorg/arcadia/contrib/libs/onnx -I/home/thegeorg/.ya/build/build_root/mykz/000195/contrib/libs/onnx -I/home/thegeorg/arcadia/contrib/libs/protobuf/src -I/home/thegeorg/arcadia/contrib/restricted/abseil-cpp -I/home/thegeorg/arcadia/contrib/restricted/boost/mp11/include --cflags --target=x86_64-linux-gnu --sysroot=/home/thegeorg/.ya/tools/v4/1966560555 -B/home/thegeorg/.ya/tools/v4/1966560555/usr/bin -fdebug-prefix-map=/home/thegeorg/.ya/build/build_root/mykz/000195=/-B -Xclang -fdebug-compilation-dir -Xclang /tmp -pipe -m64 -O3 -g -ggnu-pubnames -fexceptions -fno-common -fuse-init-array -fcolor-diagnostics -faligned-allocation -fdebug-default-version=4 -ffunction-sections -fdata-sections -w -DFAKEID=10157985 -DARCADIA_ROOT=/home/thegeorg/arcadia -DARCADIA_BUILD_ROOT=/home/thegeorg/.ya/build/build_root/mykz/000195 -D_THREAD_SAFE -D_PTHREADS -D_REENTRANT -D_LIBCPP_ENABLE_CXX17_REMOVED_FEATURES -D_LARGEFILE_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_YNDX_LIBUNWIND_ENABLE_EXCEPTION_BACKTRACE -UNDEBUG -D__LONG_LONG_SUPPORTED -DSSE_ENABLED=1 -DSSE3_ENABLED=1 -DSSSE3_ENABLED=1 -DSSE41_ENABLED=1 -DSSE42_ENABLED=1 -DPOPCNT_ENABLED=1 -DCX16_ENABLED=1 -DCPUINFO_SUPPORTED -DCPUINFO_SUPPORTED_PLATFORM=1 -DEIGEN_MPL2_ONLY -DEIGEN_USE_THREADS -DENABLE_CPU_FP16_TRAINING_OPS -DNSYNC_ATOMIC_CPP11 -DONNX_ML=1 -DONNX_NAMESPACE=onnx -DONNX_USE_LITE_PROTO=1 -DORT_ENABLE_STREAM -DPLATFORM_POSIX -DPROTOBUF_USE_DLLS -DUSE_CUDA=1 -D__ONNX_NO_DOC_STRINGS -D_libunwind_ -nostdinc++ -DLIBCXX_BUILDING_LIBCXXRT -I/home/thegeorg/.ya/tools/v4/2410761119/include -DEIGEN_MAX_ALIGN_BYTES=32 -DEIGEN_MPL2_ONLY -DUSE_CUDNN=70605 -DONNX_ML=1 -DONNX_NAMESPACE=onnx -fPIC -msse2 -msse3 -mssse3 -msse4.1 -msse4.2 -mpopcnt -mcx16 -fPIC -std=c++20 -Wno-everything -nostdinc++ -std=c++14 failed with exit code 1 in /home/thegeorg/.ya/build/build_root/mykz/000195
/home/thegeorg/arcadia/contrib/libs/onnx_runtime/onnxruntime/contrib_ops/cuda/math/fft_ops_impl.cu(2): error: this declaration has no storage class or type specifier

/home/thegeorg/arcadia/contrib/libs/cxxsupp/libcxx/include/__algorithm/unwrap_iter.h(21): error: expected a ";"

The snippet is full of internals but I decided not to strip them out.
We are using nvcc 11.4 with clang 11 used to compile the host part of the .cu-file.

I am unable to reproduce the problem during nixpkgs build, only my local build fails.
As this PR does not break anything, I suggest merging it.

UTF8-BOM is not in favor any more.

@baijumeswani
Copy link
Contributor

Just confirmed that UTF-8 BOM exists in that file. I think it is reasonable to remove it from this source file. @RyanUnderhill let me know if you have any concerns.

@baijumeswani
Copy link
Contributor

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Linux QNN CI Pipeline

@baijumeswani
Copy link
Contributor

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, onnxruntime-python-checks-ci-pipeline, ONNX Runtime React Native CI Pipeline, Windows ARM64 QNN CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@snnn
Copy link
Contributor

snnn commented Apr 6, 2023

My two cents:
Most C/C++ compilers support BOM very well. When possible, you should add BOM to every C/C++ file instead of removing them. Because when a compiler sees such a thing, it knows this file is encoded in UTF-8 and will not try to use another encoding to decode the file. People are less likely to hit encoding issues on Linux because the Linux user world is dominated by English and UTF-8. But it doesn't mean there isn't such an issue. No matter the build happens on Windows or Linux, to ensure a correct encoding was used, should:

  1. Either all files have BOM
  2. Or all build commands have explicitly specified file coding by using a compiler flag like "/utf8" or "-finput-charset=utf-8".

However, most OSS projects chose to do nothing and many of them are only compliable in English UTF-8 environments. That's not a good thing. I do not against this change. But I want to raise the awareness that if you do nothing about it, by default the source files will be compiled based the current user's locale setting, and on Windows usually it's wrong because it would not be "UTF-8".

@baijumeswani baijumeswani merged commit 6557902 into microsoft:main Apr 6, 2023
@georgthegreat georgthegreat deleted the remove-bom branch April 7, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants