Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Implement Sse2 memory fence instructions#16262

Merged
tannergooding merged 2 commits into
dotnet:masterfrom
dotnetrt:sse2memory
Feb 8, 2018
Merged

Implement Sse2 memory fence instructions#16262
tannergooding merged 2 commits into
dotnet:masterfrom
dotnetrt:sse2memory

Conversation

@4creators
Copy link
Copy Markdown

@4creators 4creators commented Feb 7, 2018

No description provided.

tannergooding
tannergooding reviewed Feb 7, 2018
View reviewed changes
Comment thread src/jit/hwintrinsiccodegenxarch.cpp
}

case NI_SSE2_LoadFence:
case NI_SSE2_MemoryFence:
Copy link
Copy Markdown
Member

@tannergooding tannergooding Feb 7, 2018

Choose a reason for hiding this comment

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

nit: It seems better to just duplicate the asserts here and have the cases handled separately.

Copy link
Copy Markdown
Author

@4creators 4creators Feb 7, 2018

Choose a reason for hiding this comment

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

Agree, it will be at least couple of cycles less

@4creators
Copy link
Copy Markdown
Author

4creators commented Feb 7, 2018

test Windows_NT x64 Checked jitincompletehwintrinsic
test Windows_NT x64 Checked jitx86hwintrinsicnoavx
test Windows_NT x64 Checked jitx86hwintrinsicnoavx2
test Windows_NT x64 Checked jitx86hwintrinsicnosimd
test Windows_NT x64 Checked jitnox86hwintrinsic

test Windows_NT x86 Checked jitincompletehwintrinsic
test Windows_NT x86 Checked jitx86hwintrinsicnoavx
test Windows_NT x86 Checked jitx86hwintrinsicnoavx2
test Windows_NT x86 Checked jitx86hwintrinsicnosimd
test Windows_NT x86 Checked jitnox86hwintrinsic

test Ubuntu x64 Checked jitincompletehwintrinsic
test Ubuntu x64 Checked jitx86hwintrinsicnoavx
test Ubuntu x64 Checked jitx86hwintrinsicnoavx2
test Ubuntu x64 Checked jitx86hwintrinsicnosimd
test Ubuntu x64 Checked jitnox86hwintrinsic

test OSX10.12 x64 Checked jitincompletehwintrinsic
test OSX10.12 x64 Checked jitx86hwintrinsicnoavx
test OSX10.12 x64 Checked jitx86hwintrinsicnoavx2
test OSX10.12 x64 Checked jitx86hwintrinsicnosimd
test OSX10.12 x64 Checked jitnox86hwintrinsic

@4creators
Copy link
Copy Markdown
Author

4creators commented Feb 8, 2018

test Windows_NT x86 Checked jitx86hwintrinsicnosimd

@4creators
Copy link
Copy Markdown
Author

4creators commented Feb 8, 2018 •
edited
Loading

@tannergooding The above build timed out. In the other build there are some unrelated errors. Could you have a look and verify what is going on. I see quite a bit of random test failures in several runs in #16237 and in this PR.

https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x64_checked_windows_nt_jitx86hwintrinsicnoavx2_prtest/79/

@4creators
Copy link
Copy Markdown
Author

4creators commented Feb 8, 2018 •
edited
Loading

Windows_NT x86 Checked Build and Test (Jit - EnableIncompleteISAClass=1 FeatureSIMD=0) timed out again

https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x86_checked_windows_nt_jitx86hwintrinsicnosimd_prtest/85/

@4creators
Copy link
Copy Markdown
Author

4creators commented Feb 8, 2018

test Windows_NT x64 Checked jitx86hwintrinsicnoavx2
test Windows_NT x86 Checked jitx86hwintrinsicnosimd

@4creators
Copy link
Copy Markdown
Author

4creators commented Feb 8, 2018

Failures here are due to Test Infrastructure Failure: The paging file is too small for this operation to complete

@4creators
Copy link
Copy Markdown
Author

4creators commented Feb 8, 2018

@tannergooding @fiigii @CarolEidt PTAL

The only failure is:

D:\j\workspace\x64_checked_w---9f07846f\bin\tests\Windows_NT.x64.Checked\TestWrappers\JIT.SIMD\JIT.SIMD.XUnitWrapper.cs(177): error : JIT_SIMD._AbsGeneric_r_AbsGeneric_r_._AbsGeneric_r_AbsGeneric_r_cmd [FAIL] [D:\j\workspace\x64_checked_w---9f07846f\tests\runtest.proj]
02:45:35.577         Not enough storage is available to process this command.
02:45:35.577         The syntax of the command is incorrect.

after which there other failures with 02:42:50.560 The system is out of environment space. messege appearing between them

https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x64_checked_windows_nt_jitx86hwintrinsicnoavx2_prtest/79

CarolEidt
CarolEidt approved these changes Feb 8, 2018
View reviewed changes
Copy link
Copy Markdown

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM; I would like to see some minor changes, but would not hold up the merge for them.
@tannergooding could you review as well?

Comment thread src/jit/hwintrinsiccodegenxarch.cpp
var_types targetType = node->TypeGet();
var_types baseType = node->gtSIMDBaseType;
instruction ins = Compiler::insOfHWIntrinsic(intrinsicID, baseType);
instruction ins = INS_invalid;
Copy link
Copy Markdown

@CarolEidt CarolEidt Feb 8, 2018

Choose a reason for hiding this comment

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

This is fine, but from a stylistic perspective, since this is only used inside a subset of the switch cases, one might move the declaration down into those scopes.

Comment thread src/jit/hwintrinsicxarch.cpp
var_types baseType = TYP_UNKNOWN;
var_types retType = TYP_UNKNOWN;

assert((simdSize == 16) || (simdSize == 0));
Copy link
Copy Markdown

@CarolEidt CarolEidt Feb 8, 2018

Choose a reason for hiding this comment

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

This could use a comment; I think that whenever we do the "simdSize == 0" check it deserves a comment, e.g. "A simdSize of zero means that it is not a SIMD type" because it could be confusing.

Copy link
Copy Markdown
Member

@tannergooding tannergooding Feb 8, 2018

Choose a reason for hiding this comment

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

The SSE code added:

// The Prefetch and StoreFence intrinsics don't take any SIMD operands
// and have a simdSize of 0

tannergooding
tannergooding reviewed Feb 8, 2018
View reviewed changes
Comment thread src/jit/emitxarch.cpp
// special case sfence and the prefetch instructions as they never take a VEX prefix
if ((ins == INS_vzeroupper) || (ins == INS_sfence) || (ins == INS_prefetcht0) || (ins == INS_prefetcht1) ||
(ins == INS_prefetcht2) || (ins == INS_prefetchnta))
// special case (l|m|s)fence and the prefetch instructions as they never take a VEX prefix
Copy link
Copy Markdown
Member

@tannergooding tannergooding Feb 8, 2018

Choose a reason for hiding this comment

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

nit: We could probably just say special case the fencing and prefetch instructions...

@tannergooding
Copy link
Copy Markdown
Member

tannergooding commented Feb 8, 2018

@4creators, let us know if you are going to fix up in this PR, or in a follow up PR, so we know when to merge.

@4creators
Copy link
Copy Markdown
Author

4creators commented Feb 8, 2018

@tannergooding I would prefer to fix this in next PR due to rerunning tests burden - I am working on all remaining Sse2 in #15777 right now and would prefer to fix it there.

@tannergooding tannergooding merged commit a526598 into dotnet:master Feb 8, 2018
@tannergooding
Copy link
Copy Markdown
Member

tannergooding commented Feb 8, 2018

Thanks for letting us know. Merged.

@4creators 4creators deleted the sse2memory branch February 8, 2018 20:45
@4creators 4creators mentioned this pull request Feb 9, 2018
Merged
@4creators 4creators mentioned this pull request Mar 6, 2018
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@CarolEidt CarolEidt CarolEidt approved these changes

@tannergooding tannergooding tannergooding approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@4creators 4creators

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@4creators @tannergooding @CarolEidt