-
Notifications
You must be signed in to change notification settings - Fork 282
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
AVX512 version for POVRAY #452
base: release/v3.8.0
Are you sure you want to change the base?
AVX512 version for POVRAY #452
Conversation
For easier reference of the performance results a excel workbook detailing the same has been attached here (also part of the branch submitted) - |
Curious as to why exactly you say cmedit64 is "incompatible". Could you clarify? That aside, there is absolutely no reason to confine the build to the console version, DLL issues or no DLL issues. Just rename or delete the editor DLL's and fire up pvengine as it has been explicitly designed to work with or without the editor. |
This is quite an extensive patch. The @Srihari-mcw account profile is quite bare and as of the time of writing shows nothing about you, your background, or where you work. In addition there seems little activity apart from your threading issue PR and a few commits here and there. I need to know whether you coded this personally, are submitting it for someone who did, and whether it was a "work for hire". Putting it another way: who exactly owns the intellectual property over the code being submitted in this pull request? With a patch this extensive I need to work that out before spending any more time on it. Note: 3rd-party ownership of the code isn't bad, we have accepted similar patches before, but on those occasions we already knew who we were dealing with. |
Hi @chris20 , This was work done as a contractor. We have confirmed this code is provided license free. We will try to check on the DLLs and get back. Thanks |
Sorry but "confirmed this code is provided license-free" can't work in this case. Intellectual property generated as a "work for hire" has a specific meaning within the US copyright system. The IP belongs to the entity that paid for the work to be done. For this patch to even be considered for acceptance I must know the identity of the IP owner and must communicate directly with them. I don't understand the apparent secrecy regarding this work. It seems to be the product of significant effort. I am supposing this was sponsored by either AMD or Intel (probably the former). And if it was, that's perfectly OK. We've accepted similar patches in the past, but we were dealing directly with the copyright owner. So please cut the secrecy and just tell us who sponsored the work and how we may contact them. |
@chris20 , There is no intentional secrecy here, but we are bound by an NDA unless we receive explicit permission to share contract details. We don't anticipate any confidentiality or license issues, and have requested our client to reach out directly or comment here. Thanks |
@chris20 -- @Srihari-mcw works in my team. Feel free to reach out to me if you need any further clarifications: anshu.arya (at) amd |
Recently some testing was carried out with this patch on AMD Granite Ridge 9600X(MSVC Console Version). Significant improvements were seen with benchmark and mediasky examples especially (26% and 21%) respectively. Attaching sheet and details here for reference |
35% improvement on the benchmark is fairly impressive I will admit. |
@chris20 , some updates were made in the numbers above after further checks. The updated numbers have benchmark giving good gains around 26%. The same has been updated in the comment shared earlier. Thanks |
Hi @chris20 , Post your comments on the GUI Version, we tried to build the GUI Version. Currently its generating an executable (pvengine-avx.exe) and currently we are unable to run it. Kindly share your thoughts here, thanks. Screenshot of the folder with build is attached here We installed from your release package in the repo and were able to run the GUI Version. We tried to remove the DLLs (cmedit32/64.dll , povcmax32/64.dll) from the executable path and were able to run it successfully Also, have you tried to build with the latest changes with VS2022 for CLI/GUI Versions? Thanks |
Hi Srihari, When you say you're "unable to run it", could you please be more specific? I haven't had a chance to build the code yet. I will drop an email to Anshu's AMD address with regard to the copyright assignment. |
Hi @chris20 , Currently with the generated pvengine executable, on clicking there is no activity seen/ no GUI window opens up. Thanks |
I don't have the current configuration you're building from and even if I did, I would expect someone who has raised an 80-file PR should at least make an attempt to solve simple issues like this by themselves. If it were some deep, gnarly issue that would require the attention of one of the primary developers and is in scope for the project it would be a different issue. But, c'mon, man, ... "unable to run it" ??? ¯\(ツ)/¯ :-) Here's a hint: have you thought of using the debugger? |
Yeah @chris20 , actually we are also looking for a fix for this on our side. Just wanted to check with you for any possible inputs. Thanks |
The current PR implements AVX512 version of POV-RAY. This contains optimized implementations for Noise and DNoise functions.
Details of implementation:
We are able to observe significantly better performance for POVRAY pipeline with the AVX512 implementation.
Results related to performance comparison of AVX512 version and AVX2FMA3 version with MSVC compiler are added in avx512_build_setup folder in a xlsx file.
The results are recorded in Intel I5-1035 G1 and AMD 7600X.
README related to the AVX512 version is attached in the avx512_build_setup folder detailing how to build the Windows and UNIX versions. Details with the Windows build is attached here also for easier reference
Notes for AVX512 Windows build
==============================
The visual studio version was updated from vs2015 to vs2022 to enable support of AVX512 Version
file
build.h
(source/base/build.h) listed within it.In it replace with nameand email of person who builds the code in
BUILT_BY
flag and comment the #error directive (line 129)The AVX512 version was developed with the console version.
The GUI build has been skipped in the solution file.
Note: (Presently with the updated code the GUI project is skipped for building,
as the cmedit64.dll and povcmax64.dll from official windows distribution are
incompatible with VS2022. The console version alone is available to build and test).