PlummersSoftwareLLC / Primes

Prime Number Projects in C#/C++/Python
https://plummerssoftwarellc.github.io/PrimeView/
2.43k stars 574 forks source link

Feature primecpp march native #964

Closed DanielAtCosmicDNA closed 6 months ago

DanielAtCosmicDNA commented 6 months ago

Description

Based on PrimeCPP solution 3 but with -march=native instead of -mtune=native compiler switch.

Contributing requirements

rbergen commented 6 months ago

@DanielAtCosmicDNA There's a couple of problems with this PR:

  1. It seems to touch a whole lot of files that have nothing to do with the change you're trying to make; the "Files changed" tab shows 28 changed files. It looks to me like the changes in the last two PRs that were merged into drag-race are also included in this one. I see you rebased and force-pushed, and I think the end result is just not what you intended.
  2. (And this is the main one:) a relatively minor change in a compiler flag is not a reason to add a new solution. If you want to propose a change to it you can open a PR to change the existing solution. I will then try to get the original author of the solution to respond to it, and decide myself on the merit of the change if they don't.
DanielAtCosmicDNA commented 6 months ago

The resulting code is what I intended it to be, the addition of solution 5 with a more aggressive optimization switch than the original solution. I just rebased it to make it easier for the project to merge considering most recent updates that were not present when I started working on the base of this PR.

This solution is supposed to be statistically faster than the version with mtune. That is the reason why I added it as a separate solution, it would be nice to see how they fare when running dude by side in different machines.

DanielAtCosmicDNA commented 6 months ago

I can only see 7 different files on my end.

rbergen commented 6 months ago

Yes, now there are only 7 files. I commented on the PR as it stood before your last commit, and at that time there were 28.

I compared the actual implementation (i.e. the .cpp file) in C++ solution 3 and your proposed solution 5, and they are identical - except for you changing the ownership of the solution in the latter. Which still boils down to the solution you propose to add being a duplicate of solution 3, and we simply won't merge that.

If there is a compile flag optimization you think would make PrimeCPP/solution_3 execute faster, then you can open a PR to modify that flag in said solution, as I indicated in my previous comment. I will handle such a PR as I also indicated - as I have similar ones in the past.

DanielAtCosmicDNA commented 6 months ago

Yes, now there are only 7 files. I commented on the PR as it stood before your last commit, and at that time there were 28.

I compared the actual implementation (i.e. the .cpp file) in C++ solution 3 and your proposed solution 5, and they are identical - except for you changing the ownership of the solution in the latter. Which still boils down to the solution you propose to add being a duplicate of solution 3, and we simply won't merge that.

If there is a compile flag optimization you think would make PrimeCPP/solution_3 execute faster, then you can open a PR to modify that flag in said solution, as I indicated in my previous comment. I will handle such a PR as I also indicated - as I have similar ones in the past.

That is perfectly reasonable and understandable. The only drawback is we will not be able to statistically compare version 3 with version 5.

rbergen commented 6 months ago

That's a fair point in itself, but this project is not about running a drag race between different versions of identical implementations. The drag race primarily concerns different languages, and secondarily different approaches within each of those. If you can improve the execution speed of PrimeCPP/solution_3 then you're doing that approach in that language a favor, in comparison with all the other languages(' approaches). That in itself is a contribution to the goal of the project.

davepl commented 6 months ago

If the compiler flag makes a difference, please add it to solution_3 if it makes a measurable improvement!

If the other changes -also- improve it further, then it makes sense for them to be their own solution, along with the switch. But all CPP projects that benefit from it where it can be trivially added should have it, since we're racing languages and not flags :-)

DanielAtCosmicDNA commented 6 months ago

I am closing this pull request as the -march=native switch might be used to strengthen all present solutions being compiled by Clang++. This way we might be able to strengthen the C++ language horse. It would be nice to reach consensus before such this change is merged though.