Fix codegen for StoreNonTemporal#23511
Conversation
| op2Reg = op2->gtRegNum; | ||
| instruction ins = HWIntrinsicInfo::lookupIns(intrinsicId, baseType); | ||
| op1Reg = op1->gtRegNum; | ||
| GenTreeStoreInd store = storeIndirForm(node->TypeGet(), op1, op2); |
There was a problem hiding this comment.
You no longer need to set op1Reg or op2Reg here ?
Also, add some asserts and mark some intrinsics as not supporting containment. Fix #23509
| @@ -497,7 +497,7 @@ HARDWARE_INTRINSIC(AVX2_HorizontalSubtractSaturate, "HorizontalS | |||
| HARDWARE_INTRINSIC(AVX2_InsertVector128, "InsertVector128", AVX2, -1, 32, 3, {INS_vinserti128, INS_vinserti128, INS_vinserti128, INS_vinserti128, INS_vinserti128, INS_vinserti128, INS_vinserti128, INS_vinserti128, INS_invalid, INS_invalid}, HW_Category_IMM, HW_Flag_FullRangeIMM) | |||
| HARDWARE_INTRINSIC(AVX2_LoadAlignedVector256NonTemporal, "LoadAlignedVector256NonTemporal", AVX2, -1, 32, 1, {INS_movntdqa, INS_movntdqa, INS_movntdqa, INS_movntdqa, INS_movntdqa, INS_movntdqa, INS_movntdqa, INS_movntdqa, INS_invalid, INS_invalid}, HW_Category_MemoryLoad, HW_Flag_NoFlag) | |||
| HARDWARE_INTRINSIC(AVX2_MaskLoad, "MaskLoad", AVX2, -1, 0, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_vpmaskmovd, INS_vpmaskmovd, INS_vpmaskmovq, INS_vpmaskmovq, INS_invalid, INS_invalid}, HW_Category_MemoryLoad, HW_Flag_UnfixedSIMDSize) | |||
There was a problem hiding this comment.
Does MaskLoad also need to be marked NoContainment for now?
There was a problem hiding this comment.
No, it's explicitly handled here in Lowering: https://github.com/dotnet/coreclr/blob/master/src/jit/lowerxarch.cpp#L2952 and here in CodeGen: https://github.com/dotnet/coreclr/blob/master/src/jit/hwintrinsiccodegenxarch.cpp#L184.
However, as @fiigii pointed out, we need to expand our testing to cover more containment scenarios.
| case NI_BMI2_X64_MultiplyNoFlags: | ||
| { | ||
| // These do not support containment | ||
| assert(!op2->isContained()); |
There was a problem hiding this comment.
Many of these are just temporary, correct? That is we should be able to make them containable eventually and change emitIns_AR_R into emitInsStoreInd (in a separate PR)?
There was a problem hiding this comment.
Yes, it should be possible, but I didn't want to tackle that complexity at this point.
|
@dotnet/dnceng could somebody look at this PR? 9 build jobs are shown as pending but that have not started according to ADO: There are many free machines in each build pool and there are succesful build tasks there were run later. |
|
@dotnet/jit-contrib @dotnet/dnceng - any recommendations for how to get the tests completed? (Or merge without waiting for completion), as this is fixing an existing test failure? |
|
@CarolEidt looking now... |
|
@CarolEidt @mmitche just got back from some training, and I'll talk with him about it. I can't find evidence of anything going wrong from the telemetry we have, both from the pool provider and Helix. We'll keep investigating, I'd say if the tests pass in a couple places just merge and if this keeps happening let me know. |
* Fix codegen for StoreNonTemporal Also, add some asserts and mark some intrinsics as not supporting containment. Fix dotnet/coreclr#23509 Commit migrated from dotnet/coreclr@ffe8a33
Also, add some asserts and mark some intrinsics as not supporting containment.
Fix #23509