Skip to content

Optimize Vector128 and Vector256.Create methods #35857

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.

Sign up for GitHub

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

Merged
tannergooding merged 14 commits into from
May 8, 2020
Merged

Optimize Vector128 and Vector256.Create methods #35857

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
97f40a4
Updating Vector128.Create(T value) to be intrinsic
tannergooding May 5, 2020
f89e85f
Updating Vector128.Create(T, ..., T) to be intrinsic
tannergooding May 5, 2020
0056c5f
Updating Vector256.Create(T) and Vector256.Create(T, ..., T) to be in…
tannergooding May 5, 2020
8a5a12e
Applying formatting patch
tannergooding May 5, 2020
ef8ec3f
Merge remote-tracking branch 'dotnet/master' into const-vector
tannergooding May 6, 2020
fdf297f
Adding additional comments explaining how the Vector128.Create and Ve…
tannergooding May 7, 2020
9376428
Add an assert that argCnt is as expected for LowerHWIntrinsicCreate
tannergooding May 7, 2020
96d9199
Applying formatting patch
tannergooding May 7, 2020
781cc04
Use unsigned rather than signed types in LowerHWIntrinsicCreate
tannergooding May 7, 2020
c90baf3
Have HWIntrinsicInfo::lookupId take the number of arguments into account
tannergooding May 7, 2020
6f3511b
Merge remote-tracking branch 'dotnet/master' into const-vector
tannergooding May 7, 2020
67f1b1f
Adjusting Vector128.Create(T, ..., T) to not be recursive on ARM64
tannergooding May 7, 2020
c056b88
Fixing MyICJI::allocMem in superpmi to respect certain CorJitAllocMem…
tannergooding May 8, 2020
51c7d28
Avoid an implicit downcast
tannergooding May 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Updating Vector128.Create(T value) to be intrinsic
  • Loading branch information
@tannergooding
tannergooding committed May 5, 2020
commit 97f40a40184a26572de14a90eb22fb70cef970d5
1 change: 1 addition & 0 deletions src/coreclr/src/jit/hwintrinsiclistxarch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ HARDWARE_INTRINSIC(Vector128, AsVector2,
HARDWARE_INTRINSIC(Vector128, AsVector3, 16, 1, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_Helper, HW_Flag_SpecialImport|HW_Flag_NoContainment|HW_Flag_BaseTypeFromFirstArg|HW_Flag_NoRMWSemantics)
HARDWARE_INTRINSIC(Vector128, AsVector4, 16, 1, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_Helper, HW_Flag_SpecialImport|HW_Flag_NoContainment|HW_Flag_BaseTypeFromFirstArg|HW_Flag_NoRMWSemantics)
HARDWARE_INTRINSIC(Vector128, AsVector128, 16, 1, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_Helper, HW_Flag_SpecialImport|HW_Flag_NoContainment|HW_Flag_BaseTypeFromFirstArg|HW_Flag_NoRMWSemantics)
HARDWARE_INTRINSIC(Vector128, Create, 16, -1, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_Helper, HW_Flag_SpecialImport|HW_Flag_SpecialCodeGen|HW_Flag_NoRMWSemantics)
HARDWARE_INTRINSIC(Vector128, CreateScalarUnsafe, 16, 1, {INS_mov_i2xmm, INS_mov_i2xmm, INS_mov_i2xmm, INS_mov_i2xmm, INS_mov_i2xmm, INS_mov_i2xmm, INS_mov_i2xmm, INS_mov_i2xmm, INS_movss, INS_movsdsse2}, HW_Category_SIMDScalar, HW_Flag_SpecialImport|HW_Flag_SpecialCodeGen|HW_Flag_NoRMWSemantics)
// The instruction generated for float/double depends on which ISAs are supported
HARDWARE_INTRINSIC(Vector128, get_AllBitsSet, 16, 0, {INS_pcmpeqd, INS_pcmpeqd, INS_pcmpeqd, INS_pcmpeqd, INS_pcmpeqd, INS_pcmpeqd, INS_pcmpeqd, INS_pcmpeqd, INS_cmpps, INS_cmppd}, HW_Category_Helper, HW_Flag_SpecialImport|HW_Flag_NoContainment|HW_Flag_NoRMWSemantics)
Expand Down
57 changes: 57 additions & 0 deletions src/coreclr/src/jit/hwintrinsicxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,7 @@ GenTree* Compiler::impBaseIntrinsic(NamedIntrinsic intrinsic,
{
GenTree* retNode = nullptr;
GenTree* op1 = nullptr;
GenTree* op2 = nullptr;

if (!featureSIMD)
{
Expand Down Expand Up @@ -747,6 +748,62 @@ GenTree* Compiler::impBaseIntrinsic(NamedIntrinsic intrinsic,
break;
}

case NI_Vector128_Create:
{
#if defined(TARGET_X86)
if (varTypeIsLong(baseType))
{
// TODO-XARCH-CQ: It may be beneficial to emit the movq
// instruction, which takes a 64-bit memory address and
// works on 32-bit x86 systems.
break;
Copy link
Contributor

@CarolEidt CarolEidt May 6, 2020

Choose a reason for hiding this comment

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

Is there a reason you chose not to do this?

Copy link
Member Author

@tannergooding tannergooding May 6, 2020

Choose a reason for hiding this comment

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

It's not something we are handling today anywhere else and we have an identical TODO in the other places.

I believe this would just require us creating a GT_IND node since x86 requires it be a long* or ulong*, but I'm not positive if that is the ideal/correct way to handle it

Copy link
Member Author

@tannergooding tannergooding May 6, 2020

Choose a reason for hiding this comment

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

Just noting, this is also not something we are handling in the managed implementation today.

Copy link
Contributor

@CarolEidt CarolEidt May 6, 2020

Choose a reason for hiding this comment

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

Right, I gathered that, but was just curious, given the comment.

Copy link
Member Author

@tannergooding tannergooding May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not super familiar with the differences between the different addressing forms in the JIT and when each is appropriate to use.

Do we have a helper function that will create a T* or ref T from an arbitrary T (In this case a ulong* from a ulong) when the given operand on the stack could be a constant, local, field, byref, or other indirection?

Copy link
Contributor

@CarolEidt CarolEidt May 6, 2020

Choose a reason for hiding this comment

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

IIUC you really want a GT_ADDR, to get the address of an operand. I the JIT addresses are not strongly typed, they are always TYP_BYREF, so it would just be something like:

GenTree* addr = gtNewOperNode(GT_ADDR, TYP_BYREF, op);

Copy link
Contributor

@CarolEidt CarolEidt May 6, 2020

Choose a reason for hiding this comment

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

Though I'd note that I'm not really pressing for you to do this now - I was just curious why you added the comment rather than doing it.

Copy link
Member Author

@tannergooding tannergooding May 6, 2020

Choose a reason for hiding this comment

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

I'll attempt this in a follow up PR, hopefully it is that straightforward 😄

}
#endif // TARGET_X86

// We shouldn't handle this as an intrinsic if the
// respective ISAs have been disabled by the user.

if (baseType == TYP_FLOAT)
{
if (!compExactlyDependsOn(InstructionSet_SSE))
{
break;
}
}
else if (!compExactlyDependsOn(InstructionSet_SSE2))
{
break;
}

if (sig->numArgs == 1)
{
op1 = impPopStack().val;
retNode = gtNewSimdHWIntrinsicNode(retType, op1, intrinsic, baseType, simdSize);
}
else if (sig->numArgs == 2)
{
op2 = impPopStack().val;
op1 = impPopStack().val;
retNode = gtNewSimdHWIntrinsicNode(retType, op1, op2, intrinsic, baseType, simdSize);
}
else
{
assert(sig->numArgs >= 3);

GenTreeArgList* tmp = nullptr;

for (unsigned i = 0; i < sig->numArgs; i++)
{
tmp = gtNewArgList(impPopStack().val);
tmp->gtOp2 = op1;
op1 = tmp;
}

retNode = gtNewSimdHWIntrinsicNode(retType, op1, intrinsic, baseType, simdSize);
}
break;
}

case NI_Vector128_CreateScalarUnsafe:
{
assert(sig->numArgs == 1);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/jit/lower.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ class Lowering final : public Phase
#ifdef FEATURE_HW_INTRINSICS
void LowerHWIntrinsic(GenTreeHWIntrinsic* node);
void LowerHWIntrinsicCC(GenTreeHWIntrinsic* node, NamedIntrinsic newIntrinsicId, GenCondition condition);
void LowerHWIntrinsicCreate(GenTreeHWIntrinsic* node);
void LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node);
#endif // FEATURE_HW_INTRINSICS

Expand Down
Loading