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

AVX512 version for POVRAY #452

Open
wants to merge 3 commits into
base: release/v3.8.0
Choose a base branch
from

Conversation

Srihari-mcw
Copy link

The current PR implements AVX512 version of POV-RAY. This contains optimized implementations for Noise and DNoise functions.

Details of implementation:

  • Implementations/Modifications for single coordinate and double coordinate Noise/DNoise function that processes single/two Vector3D inputs at a time.
  • An implementation to process and get noise for 8 multiples of single Vector3D coordinate input at a time.
  • Rewritten versions of functions like DTurbulence/Turbulence/wrinkles/Initialize_Waves and classes like GranitePattern/WrinklesPattern so as to process multiple Vector3D coordinates at a time and use capabilities of AVX512. The present versions of these functions are also retained so that we can take that implementation route whenever we want the code to run with the other versions (Let's say AVX2FMA3)
  • Additional flags and checks were added to check if the machine supports AVX512 and corresponding code flow.

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

  1. Set the configuration in solution file to Release-AVX512 | x64
  2. Select the 'Generic POV-Ray > povbase' project and expand 'Backend Headers', then open the
    file build.h(source/base/build.h) listed within it.In it replace with name
    and email of person who builds the code in BUILT_BY flag and comment the #error directive (line 129)
  3. In syspovconfig.h(windows/povconfig/syspovconfig.h) uncomment the #define _CONSOLE. (line 56)
    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).
  4. Build the solution file and in the vs2022/bin64 folder we can run the POVRAY examples with povconsole-avx512.exe.
         General command example - povconsole-avx512.exe +Ibenchmark.pov
         Single worker thread - povconsole-avx512.exe +WT1 benchmark.pov
         Output image - benchmark.png

@Srihari-mcw
Copy link
Author

Srihari-mcw commented May 23, 2023

For easier reference of the performance results a excel workbook detailing the same has been attached here (also part of the branch submitted) -
POVRAY Compiled Results - AVX512 - AMD 7600X and Intel I5-1035 G1 with +WT1 Option.xlsx

@Srihari-mcw
Copy link
Author

Srihari-mcw commented Jun 1, 2023

Sample results for dimensions 320 x 240 :

AMD Raphael 7600X

image

image

Intel Icelake I5-1035 G1
image
image

@chris20 chris20 self-assigned this Oct 24, 2024
@chris20
Copy link
Member

chris20 commented Nov 8, 2024

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).

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.

@chris20
Copy link
Member

chris20 commented Nov 8, 2024

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.

@Srihari-mcw
Copy link
Author

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

@chris20
Copy link
Member

chris20 commented Nov 17, 2024

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.

@Srihari-mcw
Copy link
Author

Srihari-mcw commented Nov 19, 2024

@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

@anshuarya
Copy link
Contributor

@chris20 -- @Srihari-mcw works in my team. Feel free to reach out to me if you need any further clarifications: anshu.arya (at) amd

@Srihari-mcw
Copy link
Author

Srihari-mcw commented Nov 21, 2024

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

AMD 9600X - POVRay test results.xlsx

image
image

@chris20
Copy link
Member

chris20 commented Nov 23, 2024

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 (35% and 26%) respectively.

35% improvement on the benchmark is fairly impressive I will admit.

@Srihari-mcw
Copy link
Author

Srihari-mcw commented Nov 25, 2024

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 (35% and 26%) respectively.

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

@Srihari-mcw
Copy link
Author

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

povray output folder

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

@chris20
Copy link
Member

chris20 commented Dec 9, 2024

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.

@Srihari-mcw
Copy link
Author

Hi @chris20 ,

Currently with the generated pvengine executable, on clicking there is no activity seen/ no GUI window opens up. Thanks

@chris20
Copy link
Member

chris20 commented Dec 10, 2024

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?

@Srihari-mcw
Copy link
Author

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

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.

3 participants