Skip to content

Commit

Permalink
fix: index variable in OpenMP 'for' statement must have signed integr…
Browse files Browse the repository at this point in the history 
…al type
  • Loading branch information
@zdenop
zdenop committed Dec 4, 2022
1 parent 51cf430 commit bc7a7ee
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/ccmain/par_control.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ void Tesseract::PrerecAllWordsPar(const std::vector<WordData> &words) {
# pragma omp parallel for num_threads(10)
#endif // _OPENMP
// NOLINTNEXTLINE(modernize-loop-convert)
for (size_t b = 0; b < blobs.size(); ++b) {
for (auto b = 0; b < blobs.size(); ++b) {
*blobs[b].choices =
blobs[b].tesseract->classify_blob(blobs[b].blob, "par", ScrollView::WHITE, nullptr);
}
Expand Down
2 changes: 1 addition & 1 deletion src/lstm/parallel.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ bool Parallel::Backward(bool debug, const NetworkIO &fwd_deltas, NetworkScratch
#ifdef _OPENMP
# pragma omp parallel for num_threads(stack_size)
#endif
for (unsigned i = 0; i < stack_size; ++i) {
for (auto i = 0; i < stack_size; ++i) {
stack_[i]->Backward(debug, *in_deltas[i], scratch, i == 0 ? back_deltas : out_deltas[i]);
}
if (needs_to_backprop_) {
Expand Down

5 comments on commit bc7a7ee

@amitdo
Copy link
Collaborator

@amitdo amitdo commented on bc7a7ee Dec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

264dfb3

#3285

#3475

@stweil
Copy link
Member

@stweil stweil commented on bc7a7ee Dec 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit produces a compiler warning:

../../../src/lstm/parallel.cpp: In member function ‘virtual bool tesseract::Parallel::Backward(bool, const tesseract::NetworkIO&, tesseract::NetworkScratch*, tesseract::NetworkIO*)’:
../../../src/lstm/parallel.cpp:140:24: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare]
  140 |     for (auto i = 0; i < stack_size; ++i) {
      |                      ~~^~~~~~~~~~~~

Why was that change needed? I thought that there was already a solution for MSVC and OpenCL which worked with unsigned integers?

@zdenop
Copy link
Contributor Author

@zdenop zdenop commented on bc7a7ee Dec 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to simplify cmake OpenMP with MSVC + get rid off the warning "overriding '/openmp' with '/openmp:llvm'" and I finished at the stage I need this change to compile tesseract...
Neither MSVC 2019 nor clang 15 produce such a warning with this change so I conder it as safe..
We can revert this as I did not finish this change.

@amitdo
Copy link
Collaborator

@amitdo amitdo commented on bc7a7ee Dec 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Microsoft officially supports the msvc with openmp-llvm runtime combo and tells developers to use it if they need openmp>2.0, which is exactly what we did prior to this commit.

@stweil
Copy link
Member

@stweil stweil commented on bc7a7ee Dec 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the commit produces two compiler warnings with Linux autoconf builds (regression), it should be either reverted (my favorite), or we need conditional code for MSVC and other compilers.

Please sign in to comment.