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

Add support for SSE 4.2. #1710

Merged
merged 2 commits into from
Oct 10, 2021
Merged

Conversation

ActuallyaDeviloper
Copy link
Contributor

@ActuallyaDeviloper ActuallyaDeviloper commented Sep 7, 2021

What does this PR do?

This adds support for SSE 4.2 in Premake. The change is quite simple. The lack of SSE 4.2 support so far was probably just an oversight.

How does this PR change Premake's behavior?

There are no behaviour changes for already written and currently working Premake build scripts.

Anything else we should know?

I did not add new tests, since there are none for global configurations themselves and I just added new table entries and really did not change any actual logic.

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits
  • Align documentation to your changes

You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!

Copy link
Member

@nickclark2016 nickclark2016 left a comment

Choose a reason for hiding this comment

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

Please add a unit test for the value.

@ActuallyaDeviloper
Copy link
Contributor Author

Thank your for your speedy response.

Do you really think a test is necessary? There are also no tests that I can find for SSE 3, SSSE 3 or SSE 4.1 and I think that is totally fine that way.

If you insist that this must be tested, I am actually not sure how to. The only tests for SSE-like kind of things seem to be for specific toolsets. Would I need to make a new test for each of them?

@nickclark2016
Copy link
Member

I'll defer to @starkos or @samsinsane for final decision on a unit test, but it's part of our checklist.

Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

Regarding the unit tests, I guess it would depend if SSE4.2 is only supported by a subset of VS. It looks like that's how it's been tested before, I'm not sure if explicitly testing each possible value is worth the effort but given how we map the values for VS perhaps we should ensure we are mapping correctly? I think this can be merged without any unit tests though.

As for how to do the tests, just as they are already:

function suite.instructionSet_onIA32_onVS2010()
vectorextensions "IA32"
prepare()
test.isemptycapture()
end
function suite.instructionSet_onIA32()
p.action.set("vs2012")
vectorextensions "IA32"
prepare()
test.capture [[
<EnableEnhancedInstructionSet>NoExtensions</EnableEnhancedInstructionSet>
]]
end
function suite.instructionSet_onSSE()
vectorextensions "SSE"
prepare()
test.capture [[
<EnableEnhancedInstructionSet>StreamingSIMDExtensions</EnableEnhancedInstructionSet>
]]
end
function suite.instructionSet_onSSE2()
vectorextensions "SSE2"
prepare()
test.capture [[
<EnableEnhancedInstructionSet>StreamingSIMDExtensions2</EnableEnhancedInstructionSet>
]]
end
function suite.instructionSet_onAVX()
p.action.set("vs2013")
vectorextensions "AVX"
prepare()
test.capture [[
<EnableEnhancedInstructionSet>AdvancedVectorExtensions</EnableEnhancedInstructionSet>
]]
end
function suite.instructionSet_onAVX_onVS2010()
vectorextensions "AVX"
prepare()
test.isemptycapture()
end
function suite.instructionSet_onAVX2()
p.action.set("vs2013")
vectorextensions "AVX2"
prepare()
test.capture [[
<EnableEnhancedInstructionSet>AdvancedVectorExtensions2</EnableEnhancedInstructionSet>
]]
end
function suite.instructionSet_onAVX2_onVS2012()
p.action.set("vs2012")
vectorextensions "AVX2"
prepare()
test.isemptycapture()
end

function suite.cflags_onSSE()
vectorextensions "SSE"
prepare()
test.contains("/arch:SSE", msc.getcflags(cfg))
end
function suite.cflags_onSSE2()
vectorextensions "SSE2"
prepare()
test.contains("/arch:SSE2", msc.getcflags(cfg))
end
function suite.cflags_onAVX()
vectorextensions "AVX"
prepare()
test.contains("/arch:AVX", msc.getcflags(cfg))
end
function suite.cflags_onAVX2()
vectorextensions "AVX2"
prepare()
test.contains("/arch:AVX2", msc.getcflags(cfg))
end

function suite.cflags_onSSE()
vectorextensions "SSE"
prepare()
test.contains({ "-msse" }, gcc.getcflags(cfg))
end
function suite.cflags_onSSE2()
vectorextensions "SSE2"
prepare()
test.contains({ "-msse2" }, gcc.getcflags(cfg))
end
function suite.cflags_onAVX()
vectorextensions "AVX"
prepare()
test.contains({ "-mavx" }, gcc.getcflags(cfg))
end
function suite.cflags_onAVX2()
vectorextensions "AVX2"
prepare()
test.contains({ "-mavx2" }, gcc.getcflags(cfg))
end

website/docs/vectorextensions.md Outdated Show resolved Hide resolved
@ActuallyaDeviloper ActuallyaDeviloper force-pushed the master branch 2 times, most recently from ec9616c to 510f50d Compare September 10, 2021 14:10
@ActuallyaDeviloper
Copy link
Contributor Author

You are right, there are no changes regarding the VS version because VS (unfortunately) can't directly target SSE3 - SSE4.2 in any version with it's code generation controlled by these compiler switches.

I have made the AVX entry the second one again even though I am sceptical of the existing order in regards to if there is any meaning to it.

@ActuallyaDeviloper
Copy link
Contributor Author

Can we maybe make progress with this?

@nickclark2016 Apparently both @starkos and @samsinsane consider the level of preexisting testing sufficient. If I am not mistaken, your counter argument was only due to the formal requirement of fulfilling the checklist. Can you accept this patch set?

@nickclark2016
Copy link
Member

If tests are added for MSC (see test_msc.lua) and GCC (see test_gcc.lua), I'm happy to approve.

@ActuallyaDeviloper
Copy link
Contributor Author

@nickclark2016 Can we make progress with this now?

@samsinsane samsinsane merged commit b73d56a into premake:master Oct 10, 2021
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.

4 participants