Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a build warning in SparseTensor code for 32-bit build configs #18766

Merged
merged 6 commits into from
Dec 13, 2023
Merged

Fix a build warning in SparseTensor code for 32-bit build configs #18766

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
848304c
update
snnn Dec 11, 2023
049cc21
line wrapping
snnn Dec 12, 2023
6fa8cd5
Update onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc
snnn Dec 12, 2023
7cfdd65
update
snnn Dec 12, 2023
61b5dd5
update
snnn Dec 12, 2023
5b4f39c
update
snnn Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 31 additions & 7 deletions onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,37 @@
const auto& b_dims = B.Shape().GetDims();
const auto& out_dims = output.Shape().GetDims();
auto csr_view = A.AsCsr();
snnn marked this conversation as resolved.
Show resolved Hide resolved

ConstSparseMatrixMap<T> map_A(a_dims[0], a_dims[1], A.NumValues(),
csr_view.Outer().Data<int64_t>(),
csr_view.Inner().Data<int64_t>(),
const Eigen::Index* inner_index_pointer = nullptr;
const Eigen::Index* outer_index_pointer = nullptr;
std::unique_ptr<Eigen::Index[]> buffer_holder_inner, buffer_holder_outer;
if constexpr (std::is_same<Eigen::Index, int64_t>::value) {
snnn marked this conversation as resolved.
Show resolved Hide resolved
// On macOS the following reinterpret_cast is necessary because Eigen::Index is an alias of `long` but int64_t is `long long`. Though they have the same size, compilers still do not allow an implicit casting between them.

Check warning on line 91 in onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc

View workflow job for this annotation

GitHub Actions / cpplint

[cpplint] onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc#L91 

 Lines should be <= 120 characters long  [whitespace/line_length] [2]
Raw output 
onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc:91:  Lines should be <= 120 characters long  [whitespace/line_length] [2]
inner_index_pointer = reinterpret_cast<const Eigen::Index*>(csr_view.Inner().Data<int64_t>());
outer_index_pointer = reinterpret_cast<const Eigen::Index*>(csr_view.Outer().Data<int64_t>());
} else {
const size_t inner_size = narrow<size_t>(csr_view.Inner().Shape().Size());
const size_t outer_size = narrow<size_t>(csr_view.Outer().Shape().Size());
buffer_holder_inner.reset(new Eigen::Index[inner_size]);
buffer_holder_outer.reset(new Eigen::Index[outer_size]);
inner_index_pointer = buffer_holder_inner.get();
outer_index_pointer = buffer_holder_outer.get();
#ifdef _WIN32
#pragma warning(push)
// The copy below may loss precision. Ignore the warnings for now
#pragma warning(disable : 4244)
#endif
std::copy_n<const int64_t*, size_t, Eigen::Index*>(csr_view.Inner().Data<int64_t>(), inner_size, buffer_holder_inner.get());

Check warning on line 106 in onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc

View workflow job for this annotation

GitHub Actions / cpplint

[cpplint] onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc#L106 

 Lines should be <= 120 characters long  [whitespace/line_length] [2]
Raw output 
onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc:106:  Lines should be <= 120 characters long  [whitespace/line_length] [2]
snnn marked this conversation as resolved.
Show resolved Hide resolved
std::copy_n<const int64_t*, size_t, Eigen::Index*>(csr_view.Outer().Data<int64_t>(), outer_size, buffer_holder_outer.get());

Check warning on line 107 in onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc

View workflow job for this annotation

GitHub Actions / cpplint

[cpplint] onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc#L107 

 Lines should be <= 120 characters long  [whitespace/line_length] [2]
Raw output 
onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc:107:  Lines should be <= 120 characters long  [whitespace/line_length] [2]
#ifdef _WIN32
#pragma warning(pop)
#endif
}
ConstSparseMatrixMap<T> map_A(narrow<Eigen::Index>(a_dims[0]), narrow<Eigen::Index>(a_dims[1]), narrow<Eigen::Index>(A.NumValues()),

Check warning on line 112 in onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc

View workflow job for this annotation

GitHub Actions / cpplint

[cpplint] onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc#L112 

 Lines should be <= 120 characters long  [whitespace/line_length] [2]
Raw output 
onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc:112:  Lines should be <= 120 characters long  [whitespace/line_length] [2]
outer_index_pointer,
inner_index_pointer,
A.Values().Data<T>());
ConstEigenMatrixMapRowMajor<T> map_B(B.Data<T>(), b_dims[0], b_dims[1]);
EigenMatrixMapRowMajor<T> output_map(output.MutableData<T>(), out_dims[0], out_dims[1]);
ConstEigenMatrixMapRowMajor<T> map_B(B.Data<T>(), narrow<Eigen::Index>(b_dims[0]), narrow<Eigen::Index>(b_dims[1]));
EigenMatrixMapRowMajor<T> output_map(output.MutableData<T>(), narrow<Eigen::Index>(out_dims[0]), narrow<Eigen::Index>(out_dims[1]));

Check warning on line 117 in onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc

View workflow job for this annotation

GitHub Actions / cpplint

[cpplint] onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc#L117 

 Lines should be <= 120 characters long  [whitespace/line_length] [2]
Raw output 
onnxruntime/contrib_ops/cpu/math/sparse_dense_matmul.cc:117:  Lines should be <= 120 characters long  [whitespace/line_length] [2]
// XXX: Consider re-writing it as a parallel loop as Eigen requires it to use OpenMP
// XXX: Consider vectorization
SparseDenseMatMulImpl(ctx, map_A, map_B, output_map);
Expand Down Expand Up @@ -211,4 +235,4 @@
} // namespace contrib
} // namespace onnxruntime

#endif //! defined(DISABLE_SPARSE_TENSORS)
#endif //! defined(DISABLE_SPARSE_TENSORS)
2 changes: 1 addition & 1 deletion onnxruntime/core/util/math_cpuonly.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ template <typename T>
using ConstEigenMatrixMap = Eigen::Map<const Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic>>;

template <class T>
using ConstSparseMatrixMap = Eigen::Map<const Eigen::SparseMatrix<T, Eigen::RowMajor, int64_t>>;
using ConstSparseMatrixMap = Eigen::Map<const Eigen::SparseMatrix<T, Eigen::RowMajor, ptrdiff_t>>;

template <typename T>
using ConstEigenArrayMap = Eigen::Map<const Eigen::Array<T, Eigen::Dynamic, Eigen::Dynamic>>;
Expand Down
3 changes: 1 addition & 2 deletions tools/ci_build/github/azure-pipelines/linux-ci-pipeline.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,7 @@ stages:
ls $(Build.BinariesDirectory)/gccbin/bin
mkdir $(Build.BinariesDirectory)/arm32build
cd $(Build.BinariesDirectory)/arm32build
# TODO: fix the warnings and remove the --compile-no-warning-as-error arg
cmake --compile-no-warning-as-error $(Build.SourcesDirectory)/cmake -Donnxruntime_ENABLE_CPUINFO=OFF -DPython_EXECUTABLE=/usr/bin/python3 -DPYTHON_EXECUTABLE=/usr/bin/python3 -DCMAKE_BUILD_TYPE=Debug -DCMAKE_TOOLCHAIN_FILE=$(Build.SourcesDirectory)/cmake/linux_arm32_crosscompile_toolchain.cmake -G Ninja
cmake $(Build.SourcesDirectory)/cmake -Donnxruntime_ENABLE_CPUINFO=OFF -DPython_EXECUTABLE=/usr/bin/python3 -DPYTHON_EXECUTABLE=/usr/bin/python3 -DCMAKE_BUILD_TYPE=Debug -DCMAKE_TOOLCHAIN_FILE=$(Build.SourcesDirectory)/cmake/linux_arm32_crosscompile_toolchain.cmake -G Ninja
ninja
rm -rf $(Build.BinariesDirectory)/arm32build $(Build.BinariesDirectory)/gccbin
displayName: Cross-compile for Linux ARM32 and ARM64
Expand Down
Loading