Skip to content

JIT: Mark constant arguments of HWINTRINSIC nodes as DONT_CSE #63442

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
merged 10 commits into from
Jan 15, 2022
Merged

JIT: Mark constant arguments of HWINTRINSIC nodes as DONT_CSE #63442

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a7e9a87
mark constant arguments of HWINTRINSIC nodes as DONT_CSE
EgorBo Jan 6, 2022
f3de36c
limit to Create only
EgorBo Jan 6, 2022
e564ee6
Update morph.cpp
EgorBo Jan 6, 2022
c10773c
fix build
EgorBo Jan 6, 2022
c99f333
clean up
EgorBo Jan 6, 2022
28c0dfd
Merge branch 'main' of https://github.com/dotnet/runtime into dont-cs…
EgorBo Jan 12, 2022
8aeb047
Merge branch 'main' of https://github.com/dotnet/runtime into dont-cs…
EgorBo Jan 12, 2022
0a26d33
Address feedback
EgorBo Jan 12, 2022
89260c0
Update morph.cpp
EgorBo Jan 12, 2022
8f88b99
Oops. wrong one
EgorBo Jan 12, 2022
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
59 changes: 58 additions & 1 deletion src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14148,18 +14148,45 @@ GenTree* Compiler::fgMorphSmpOpOptional(GenTreeOp* tree)
GenTree* Compiler::fgMorphMultiOp(GenTreeMultiOp* multiOp)
{
gtUpdateNodeOperSideEffects(multiOp);

bool dontCseConstArguments = false;
#if defined(FEATURE_HW_INTRINSICS)
// Opportunistically, avoid unexpected CSE for hw intrinsics with IMM arguments
if (multiOp->OperIs(GT_HWINTRINSIC))
{
NamedIntrinsic hwIntrinsic = multiOp->AsHWIntrinsic()->GetHWIntrinsicId();
#if defined(TARGET_XARCH)
if (HWIntrinsicInfo::lookupCategory(hwIntrinsic) == HW_Category_IMM)
{
dontCseConstArguments = true;
}
#elif defined(TARGET_ARMARCH)
if (HWIntrinsicInfo::HasImmediateOperand(hwIntrinsic))
{
dontCseConstArguments = true;
}
#endif
}
#endif

for (GenTree** use : multiOp->UseEdges())
{
*use = fgMorphTree(*use);
multiOp->gtFlags |= ((*use)->gtFlags & GTF_ALL_EFFECT);

if (dontCseConstArguments && (*use)->OperIsConst())
{
(*use)->SetDoNotCSE();
}
}

#if defined(FEATURE_HW_INTRINSICS) && defined(TARGET_XARCH)
#if defined(FEATURE_HW_INTRINSICS)
if (opts.OptimizationEnabled() && multiOp->OperIs(GT_HWINTRINSIC))
{
GenTreeHWIntrinsic* hw = multiOp->AsHWIntrinsic();
switch (hw->GetHWIntrinsicId())
{
#if defined(TARGET_XARCH)
case NI_SSE_Xor:
case NI_SSE2_Xor:
case NI_AVX_Xor:
Expand All @@ -14185,6 +14212,36 @@ GenTree* Compiler::fgMorphMultiOp(GenTreeMultiOp* multiOp)
}
break;
}
#endif

case NI_Vector128_Create:
Copy link
Member

Choose a reason for hiding this comment

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

We should likely add back the handling (or do it in a separate PR) to have the same for any intrinsic that is HWIntrinsicInfo::lookupCategory(intrinsicId) == HW_Category_IMM (x86/x64) or HWIntrinsicInfo::HasImmediateOperand(intrinsicId) (ARM64)

Copy link
Member

Choose a reason for hiding this comment

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

In those cases, the instruction has one or more non-SIMD operands (typically a byte imm) that is functionally "required" to be a constant, as otherwise it will risk expanding to a very expensive jump table.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tannergooding could you please look one more time? I've addressed your feedback and fixed regressions - they were coming from CreateScalarUnsafe, e.g.

public float Test()
{
    return Vector128.CreateScalarUnsafe(5.0f).ToScalar()
           + Vector128.CreateScalarUnsafe(5.0f).ToScalar();
}

#if defined(TARGET_XARCH)
case NI_Vector256_Create:
#elif defined(TARGET_ARMARCH)
case NI_Vector64_Create:
#endif
{
bool hwAllArgsAreConst = true;
for (GenTree** use : multiOp->UseEdges())
{
if (!(*use)->OperIsConst())
{
hwAllArgsAreConst = false;
break;
}
}

// Avoid unexpected CSE for constant arguments for Vector_.Create
// but only if all arguments are constants.
if (hwAllArgsAreConst)
{
for (GenTree** use : multiOp->UseEdges())
{
(*use)->SetDoNotCSE();
}
}
}
break;

default:
break;
Expand Down