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.

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

forder with icc 2019/openmp 4 skips radix sort on final descent #3647

Closed
hroptatyr opened this issue Jun 18, 2019 · 3 comments · Fixed by #3648
Closed

forder with icc 2019/openmp 4 skips radix sort on final descent #3647

hroptatyr opened this issue Jun 18, 2019 · 3 comments · Fixed by #3648
Milestone

Comments

@hroptatyr
Copy link
Contributor

Intel's compiler 2019 (Version 19.0.4.243 Build 20190416) seems to be allergic to setting the value of skip (forder.c:905) by calculation for its side-effect use in forder.c:922. It's probably optimized out because the second loop contains the same body without the skip condition.

Clearly a compiler bug and I reported this issue to the Intel guys.

However, to be more explicit I propose to calculate the value of skip afterwards and separate from the key counting.

# Minimal reproducible example

Any dataset with group size >256 should do.

# Output of sessionInfo()
> sessionInfo()
R version 3.6.0 (2019-04-26)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: openSUSE Leap 42.3

Matrix products: default
BLAS/LAPACK: /opt/intel/compilers_and_libraries_2019.4.243/linux/mkl/lib/intel64_lin/libmkl_intel_lp64.so

locale:
 [1] LC_CTYPE=C.UTF-8     LC_NUMERIC=C         LC_TIME=C           
 [4] LC_COLLATE=C         LC_MONETARY=C        LC_MESSAGES=C       
 [7] LC_PAPER=C           LC_NAME=C            LC_ADDRESS=C        
[10] LC_TELEPHONE=C       LC_MEASUREMENT=C     LC_IDENTIFICATION=C 

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] data.table_1.12.3

loaded via a namespace (and not attached):
[1] compiler_3.6.0
hroptatyr added a commit to hroptatyr/data.table that referenced this issue Jun 18, 2019
Calculate `skip` condition after and separate from key counting.
@mattdowle mattdowle added this to the 1.12.4 milestone Jun 19, 2019
@mattdowle
Copy link
Member

mattdowle commented Jun 19, 2019

Thanks for the report and fix! Will merge. Do you have a link the issue you reported to Intel? This workaround will only apply to the next version of data.table so it will be useful to monitor its status so we know if Intel confirm it and which version is it fixed, so we can advise data.table users who might not be able to upgrade data.table.
Does test.data.table() fail under that Intel compiler? I hope it does fail; i.e., is test coverage ok so users know if they have this problem, or do we need to add a new test?

@mattdowle
Copy link
Member

For the news item, I've assumed test.data.table() does fail under that Intel compiler. If not, please let me know!

@hroptatyr
Copy link
Contributor Author

Yes, it does fail. That's how I noticed in the first place.

Re the report, I opened a support ticket which is not a public process. They used to have a public support forum which has been discontinued unfortunately. I'll see what I can do to make it a public issue.

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 a pull request may close this issue.

2 participants