From 156436fd58f9dd3d2b90750fd5d15005ea566e1e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 2 Apr 2021 17:09:18 +0300 Subject: [PATCH 1/3] Enable CSE for Vector_.Create --- src/coreclr/jit/valuenum.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 0525fde1492099..817e4b53b4e542 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -8918,16 +8918,16 @@ void Compiler::fgValueNumberHWIntrinsic(GenTree* tree) assert(hwIntrinsicNode != nullptr); // For safety/correctness we must mutate the global heap valuenumber - // for any HW intrinsic that performs a memory store operation + // for any HW intrinsic that performs a memory store operation if (hwIntrinsicNode->OperIsMemoryStore()) { fgMutateGcHeap(tree DEBUGARG("HWIntrinsic - MemoryStore")); } - // Check for any intrintics that have variable number of args or more than 2 args - // For now we will generate a unique value number for these cases. - // - if ((HWIntrinsicInfo::lookupNumArgs(hwIntrinsicNode->gtHWIntrinsicId) == -1) || + const bool isVariableArgs = HWIntrinsicInfo::lookupNumArgs(hwIntrinsicNode->gtHWIntrinsicId) == -1; + + // In case of functions with variable number of args we only support single-arg ones. + if ((isVariableArgs && (tree->AsOp()->gtOp2 != nullptr)) || ((tree->AsOp()->gtOp1 != nullptr) && (tree->AsOp()->gtOp1->OperIs(GT_LIST)))) { // We have a HWINTRINSIC node in the GT_LIST form with 3 or more args @@ -9018,12 +9018,12 @@ void Compiler::fgValueNumberHWIntrinsic(GenTree* tree) if (encodeResultType) { normalPair = vnStore->VNPairForFunc(tree->TypeGet(), func, op1vnp, resvnp); - assert(vnStore->VNFuncArity(func) == 2); + assert((vnStore->VNFuncArity(func) == 2) || isVariableArgs); } else { normalPair = vnStore->VNPairForFunc(tree->TypeGet(), func, op1vnp); - assert(vnStore->VNFuncArity(func) == 1); + assert((vnStore->VNFuncArity(func) == 1) || isVariableArgs); } } else From e58fd519f88e2de4cefc412cf5c2aec58b4bd264 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 5 Apr 2021 17:27:33 +0300 Subject: [PATCH 2/3] Add support for binary hw intrinsics with variable num of args --- src/coreclr/jit/valuenum.cpp | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 817e4b53b4e542..69b2b1e7098a1d 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -1900,7 +1900,9 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V assert(arg0VN != NoVN && arg1VN != NoVN); assert(arg0VN == VNNormalValue(arg0VN)); // Arguments carry no exceptions. assert(arg1VN == VNNormalValue(arg1VN)); // Arguments carry no exceptions. - assert(VNFuncArity(func) == 2); + + // Some SIMD functions with variable number of arguments are defined with zero arity + assert((VNFuncArity(func) == 0) || (VNFuncArity(func) == 2)); assert(func != VNF_MapSelect); // Precondition: use the special function VNForMapSelect defined for that. ValueNum resultVN; @@ -8924,20 +8926,18 @@ void Compiler::fgValueNumberHWIntrinsic(GenTree* tree) fgMutateGcHeap(tree DEBUGARG("HWIntrinsic - MemoryStore")); } - const bool isVariableArgs = HWIntrinsicInfo::lookupNumArgs(hwIntrinsicNode->gtHWIntrinsicId) == -1; - - // In case of functions with variable number of args we only support single-arg ones. - if ((isVariableArgs && (tree->AsOp()->gtOp2 != nullptr)) || - ((tree->AsOp()->gtOp1 != nullptr) && (tree->AsOp()->gtOp1->OperIs(GT_LIST)))) + if ((tree->AsOp()->gtOp1 != nullptr) && tree->gtGetOp1()->OperIs(GT_LIST)) { - // We have a HWINTRINSIC node in the GT_LIST form with 3 or more args - // Or the numArgs was specified as -1 in the numArgs column - - // Generate unique VN + // TODO-CQ: allow intrinsics with GT_LIST to be properly VN'ed, it will + // allow use to process things like Vector128.Create(1,2,3,4) etc. + // Generate unique VN for now. tree->gtVNPair.SetBoth(vnStore->VNForExpr(compCurBB, tree->TypeGet())); return; } + // We don't expect GT_LIST to be in the second op + assert((tree->AsOp()->gtOp2 == nullptr) || !tree->gtGetOp2()->OperIs(GT_LIST)); + VNFunc func = GetVNFuncForNode(tree); bool isMemoryLoad = hwIntrinsicNode->OperIsMemoryLoad(); @@ -8990,9 +8990,14 @@ void Compiler::fgValueNumberHWIntrinsic(GenTree* tree) #endif } + const bool isVariableNumArgs = HWIntrinsicInfo::lookupNumArgs(hwIntrinsicNode->gtHWIntrinsicId) == -1; + // There are some HWINTRINSICS operations that have zero args, i.e. NI_Vector128_Zero if (tree->AsOp()->gtOp1 == nullptr) { + // Currently we don't have intrinsics with variable number of args with a parameter-less option. + assert(!isVariableNumArgs); + if (encodeResultType) { // There are zero arg HWINTRINSICS operations that encode the result type, i.e. Vector128_AllBitSet @@ -9018,12 +9023,12 @@ void Compiler::fgValueNumberHWIntrinsic(GenTree* tree) if (encodeResultType) { normalPair = vnStore->VNPairForFunc(tree->TypeGet(), func, op1vnp, resvnp); - assert((vnStore->VNFuncArity(func) == 2) || isVariableArgs); + assert((vnStore->VNFuncArity(func) == 2) || isVariableNumArgs); } else { normalPair = vnStore->VNPairForFunc(tree->TypeGet(), func, op1vnp); - assert((vnStore->VNFuncArity(func) == 1) || isVariableArgs); + assert((vnStore->VNFuncArity(func) == 1) || isVariableNumArgs); } } else @@ -9036,12 +9041,12 @@ void Compiler::fgValueNumberHWIntrinsic(GenTree* tree) if (encodeResultType) { normalPair = vnStore->VNPairForFunc(tree->TypeGet(), func, op1vnp, op2vnp, resvnp); - assert(vnStore->VNFuncArity(func) == 3); + assert((vnStore->VNFuncArity(func) == 3) || isVariableNumArgs); } else { normalPair = vnStore->VNPairForFunc(tree->TypeGet(), func, op1vnp, op2vnp); - assert(vnStore->VNFuncArity(func) == 2); + assert((vnStore->VNFuncArity(func) == 2) || isVariableNumArgs); } } } From 8fd3d9a9f48a20ebd32a3a359375419013f4f811 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 5 Apr 2021 18:48:40 +0300 Subject: [PATCH 3/3] Make formatter happy --- src/coreclr/jit/valuenum.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 69b2b1e7098a1d..2d7e127666ba2b 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -8996,7 +8996,7 @@ void Compiler::fgValueNumberHWIntrinsic(GenTree* tree) if (tree->AsOp()->gtOp1 == nullptr) { // Currently we don't have intrinsics with variable number of args with a parameter-less option. - assert(!isVariableNumArgs); + assert(!isVariableNumArgs); if (encodeResultType) {