Update the table-driven framework to support x86 imm-intrinsics #16183

fiigii · 2018-02-02T22:58:55Z

This PR updates the table-driven framework to support x86 imm-intrinsics. After this update, implementing imm-intrinsics just needs one-line definition in hwintrinsiclistxarch.h.

This PR implements certain imm-intrinsics which have special semanctics

Avx.Compare\CompareScalar just accepts imm-value in 0-31. Out of range argument will trigger ArgumentOutOfRangeException.
Avx2\Sse2.ShiftLeftLogical\ShiftRightLogical\ShiftRightArithmetic never throw any exception
- that will be converted to shift* ymm, ymm, xmm (or shift* xmm, xmm)form when the imm-arg is not a constant. For example, Avx2.ShiftLeftLogical(v, i) would generate vmovd xmm1, edi; vpsllw ymm0, ymm0, xmm1 instead of the jumptable fallback.
- ~~ShiftLeftLogical\ShiftRightLogical will be converted to SetZeroVector256\128 on out of range imm-arg.~~
- ~~ShiftRightArithmetic's imm-arg will be normalized to the maximum valid value on out of range imm-arg.~~
~~Avx2\Sse2.ShiftLeftLogical128BitLane will be converted to SetZeroVector256\128 on out of range imm-arg.~~

fiigii · 2018-02-02T22:59:54Z

I will submit test cases later.

@CarolEidt @tannergooding @mikedn PTAL

4creators · 2018-02-02T23:31:34Z

src/jit/emitxarch.cpp

@@ -170,6 +170,14 @@ bool emitter::IsDstDstSrcAVXInstruction(instruction ins)
        case INS_psubusb:
        case INS_psubusw:
        case INS_psubw:
+        case INS_psllw:


nit: it would help if we would keep alphabetical order of instructions

Thanks, will fix.

mikedn · 2018-02-03T07:13:59Z

src/jit/emitxarch.cpp

@@ -5324,6 +5334,41 @@ void emitter::emitIns_SIMD_R_R_R(instruction ins, emitAttr attr, regNumber reg,
    }
 }

+void emitter::emitIns_SIMD_R_R_I(instruction ins, emitAttr attr, regNumber reg, regNumber reg1, int ival)
+{
+    bool isSSEShift = false;


Maybe this should be in a separate "isSseShift" function?

Yes, I will refactor here.

mikedn · 2018-02-03T07:15:44Z

src/jit/hwintrinsiccodegenxarch.cpp

+                {
+                    if (op3->IsCnsIntOrI())
+                    {
+


Extra empty line

mikedn · 2018-02-03T07:19:04Z

src/jit/hwintrinsiccodegenxarch.cpp

+                {
+                    if (op2->IsCnsIntOrI())
+                    {
+                        ssize_t ival = op2->AsIntConCommon()->IconValue();


Since you have used IsCnsIntOrI using AsIntCon would make more sense. AsIntConCommon also accepts GT_CNS_LNG which is not needed here.

Good point, thank you!

mikedn · 2018-02-03T07:25:59Z

src/jit/hwintrinsiccodegenxarch.cpp

+    assert(op2Reg != REG_NA);
+    NamedIntrinsic  intrinsicID    = node->gtHWIntrinsicId;
+    var_types       baseType       = node->gtSIMDBaseType;
+    emitAttr        attr           = (emitAttr)node->gtSIMDSize;


I don't think it makes a significant difference but in many places the EA_ATTR macro is used for this.

mikedn · 2018-02-03T07:29:23Z

src/jit/hwintrinsiccodegenxarch.cpp

+
+    // Emit the jump table
+
+    JITDUMP("\n      J_M%03u_DS%02u LABEL   DWORD\n", Compiler::s_compMethodsCount, jmpTableBase);


I don't think this is really useful. It looks like it was copied from genJumpTable but then it didn't belong there either. This kind of dump messages, if really needed, should be in emitBBTableDataGenBeg and emitDataGenData.

Or better, the data section should be included in the disassembly produced by the JIT. But that's a different story.

mikedn · 2018-02-03T07:31:04Z

src/jit/hwintrinsicxarch.cpp

+// Return Value:
+//     the max imm-value of non-full-range IMM intrinsic
+//
+int Compiler::immUpperBoundOfHWIntrinsic(NamedIntrinsic intrinsic, var_types baseType)


I just don't understand why people insist in adding more stuff to Compiler...

Hmm.. these functions are needed by multiple passes (e.g., importer, codegen, lowering, etc). Is there other better approach to organize these functions?

One reasonable option seems to be for these functions to be static members of HWIntrinsicInfo. Haven't really checked if this is actually possible or not. Another option might be global functions, similar to varType functions for example. Yet another option would be a HWIntrinsicTable class that basically acts as an abstraction for hwIntrinsicInfoArray.

That said, I'm not sure if it's worth bothering with this ATM.

mikedn · 2018-02-03T07:47:53Z

src/jit/hwintrinsicxarch.cpp

+        case NI_AVX2_ShiftLeftLogical:
+        case NI_AVX2_ShiftRightArithmetic:
+        case NI_AVX2_ShiftRightLogical:
+            return genTypeSize(baseType) * 8 - 1;


Something seems fishy here - the proper upper bound of shift is likely 255, not a number depending on the element type size.

mikedn · 2018-02-03T07:53:10Z

src/jit/hwintrinsicxarch.cpp

+GenTree* Compiler::impOutOfRangeFallback(NamedIntrinsic intrinsic, var_types simdType, var_types baseType)
+{
+    assert((flagsOfHWIntrinsic(intrinsic) & HW_Flag_OutOfRangeFallbackIMM) != 0);
+    assert(simdType == TYP_SIMD32 || simdType == TYP_SIMD16);


I see that the formatter doesn't flag these but the usual practice is to parenthesize this kind of expressions: assert((simdType == TYP_SIMD32) || (simdType == TYP_SIMD16));. Happens in multiple places.

mikedn · 2018-02-03T07:54:09Z

src/jit/hwintrinsicxarch.cpp

+            // - AND, the imm-intrinsic does not have out-of-range fallback
+
+            if (!mustExpand && lastOp->IsCnsIntOrI() &&
+                lastOp->AsIntConCommon()->IconValue() > immUpperBoundOfHWIntrinsic(intrinsic, baseType))


tannergooding · 2018-02-06T02:12:44Z

src/jit/codegenlinear.h

@@ -134,6 +134,8 @@ void genFMAIntrinsic(GenTreeHWIntrinsic* node);
 void genLZCNTIntrinsic(GenTreeHWIntrinsic* node);
 void genPCLMULQDQIntrinsic(GenTreeHWIntrinsic* node);
 void genPOPCNTIntrinsic(GenTreeHWIntrinsic* node);
+void genJumpTableFallback(


I think @sdmaclea had a good approach to this in one of his PRs.

Specifically, the codegen is essentially the same except for the actual emitted instruction and could be handled via a lambda.

https://github.com/dotnet/coreclr/pull/16085/files#diff-ddd0dded7caa705d00fda562d726c37aR5180
and
https://github.com/dotnet/coreclr/pull/16085/files#diff-ddd0dded7caa705d00fda562d726c37aR5266

4creators · 2018-02-06T16:19:37Z

src/jit/hwintrinsicxarch.cpp

+        case NI_SSE2_ShiftRightLogical128BitLane:
+            impPopStack();
+            impPopStack();
+            return gtNewSimdHWIntrinsicNode(simdType, NI_SSE2_SetZeroVector128, baseType, genTypeSize(simdType));


This can be combined with AVX2 with if / else clause for choosing NI_SSE2_SetZeroVector128 or NI_AVX_SetZeroVector256 intrinsic parameter

4creators · 2018-02-08T14:35:27Z

src/jit/hwintrinsiclistxarch.h

@@ -144,6 +144,7 @@ HARDWARE_INTRINSIC(SSE2_CompareNotLessThan,                          "CompareNot
 HARDWARE_INTRINSIC(SSE2_CompareNotLessThanOrEqual,                   "CompareNotLessThanOrEqual",                        SSE2,        6,           16,          2,            {INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_cmppd},             HW_Category_SimpleSIMD,                        HW_Flag_NoFlag)
 HARDWARE_INTRINSIC(SSE2_CompareOrdered,                              "CompareOrdered",                                   SSE2,        7,           16,          2,            {INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_cmppd},             HW_Category_SimpleSIMD,                        HW_Flag_NoFlag)
 HARDWARE_INTRINSIC(SSE2_CompareUnordered,                            "CompareUnordered",                                 SSE2,        3,           16,          2,            {INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_cmppd},             HW_Category_SimpleSIMD,                        HW_Flag_NoFlag)
+HARDWARE_INTRINSIC(SSE2_ConvertScalarToVector128Int32,               "ConvertScalarToVector128Int32",                    SSE2,       -1,           16,           1,           {INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_mov_i2xmm, INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid},           HW_Category_SimpleSIMD,                        HW_Flag_NoFlag)


It is implemented in #16237 as well

@4creators No worries, I just use this intrinsic as an internal helper. Once you implement it, I will remove here.

fiigii · 2018-02-08T19:09:58Z

src/jit/hwintrinsiccodegenxarch.cpp

+    {
+        emit->emitIns_R_I(INS_cmp, EA_4BYTE, nonConstImmReg, maxImmValue);
+        emitJumpKind jge = genJumpKindForOper(GT_GE, CK_UNSIGNED);
+        genJumpToThrowHlpBlk(jge, SCK_ARG_RNG_EXCPN);


The runtime intermittently gets "failed to find exception throw block". @CarolEidt @tannergooding @mikedn @jkotas Do you know why?

19:06:15 CLRJIT! CHECK::Trigger + 0x30E (0x7354b789) 19:06:15 CLRJIT! CodeGen::genJumpToThrowHlpBlk + 0x13C (0x73435bd7) 19:06:15 CLRJIT! CodeGen::genJumpTableFallback<<lambda_8c168ac1d3dcdd43425f08b8cdfa80f0> > + 0xF7 (0x7353e539) 19:06:15 CLRJIT! CodeGen::genHWIntrinsic + 0x2AE (0x7353ec79) 19:06:15 CLRJIT! CodeGen::genCodeForTreeNode + 0x43D (0x73516b55) 19:06:15 CLRJIT! CodeGen::genCodeForBBlist + 0x80E (0x7343936e) 19:06:15 CLRJIT! CodeGen::genGenerateCode + 0x283 (0x73434f23) 19:06:15 CLRJIT! Compiler::compCompile + 0x72D (0x73440571) 19:06:15 CLRJIT! Compiler::compCompileHelper + 0x589 (0x73441767) 19:06:15 CLRJIT! Compiler::compCompile + 0x622 (0x73440c53) 19:06:15 File: d:\j\workspace\x86_checked_w---b1af1919\src\jit\codegencommon.cpp Line: 2826 19:06:15 Image: D:\j\workspace\x86_checked_w---b1af1919\bin\tests\Windows_NT.x86.Checked\Tests\Core_Root\CoreRun.exe

{ /* Find the helper-block which raises the exception. */ Compiler::AddCodeDsc* add = compiler->fgFindExcptnTarget(codeKind, compiler->bbThrowIndex(compiler->compCurBB)); PREFIX_ASSUME_MSG((add != nullptr), ("ERROR: failed to find exception throw block")); tgtBlk = add->acdDstBlk; }

Range checks must be exposed in the importer, so that they can be appropriately modeled in the control flow. Check out the places in simd.cpp where it generates GT_SIMD_CHK.

@CarolEidt Thanks for pointing out. IMO, I still do not like the current solution to non-const (that hardcodes a jump-table fallback in the compiler), which

needs more IR to represent the range-check because the out-of-range constant argument will become a variable in the recursive function-body.

the codegen always needs to consider keeping consistent semantics with the front-end.

So I think the software switch-table fallback can significantly simplify the compiler implementation that only deals with constant imm-argument.

cc @tannergooding

I thought we determined that, we could also do this as:

result Intrinsic(op1, op2) { RangeCheck(); return Intrinsic(op1, op2); }

Which only leaves the switch table up to the JIT.

If we do it all in the JIT however, it still seems like a one time cost in implementing a helper function to do this, and it only needs to happen in the fallback case, which is already on its own codepath.

I think the software switch-table is not a problem now. In the future, if the compiler determines to optimize away the switch, we can use an attribute to disable the optimization on these intrinsics.

In the future, if the compiler determines to optimize away the switch, we can use an attribute to disable the optimization on these intrinsics.

There are multiple different compilers, and some (such as the C# compilers) don't have an option to disable optimizations for a given method. This would only work for an AOT or JIT compiler that was performing the optimization, and only if it respected the flag.

I think it would be good to better understand the specific pushback here. We already discussed this at some length in at least one of the other threads and landed on one of these two approaches:

do the range check in managed and emit the switch table in the JIT

emit the range check and the switch table in the JIT

As far as I'm concerned, it is a non-starter to consider introducing a throw in codegen that has not been introduced into the IR earlier. In addition to the technical infeasibility, it would render it impossible to apply range-check optimizations to eliminate it.

I am mildly in favor of emitting both the range check and the switch table in the JIT, primarily because it doesn't rely on the IL implementation and the JIT to both (and separately) specify the appropriate range values.

fiigii · 2018-02-21T00:41:31Z

Rebased and resolved a conflict from stack-level-setter.

tannergooding · 2018-02-21T00:43:58Z

src/jit/codegenxarch.cpp

@@ -3756,7 +3759,10 @@ void CodeGen::genCodeForCmpXchg(GenTreeCmpXchg* tree)
 // generate code for BoundsCheck nodes
 void CodeGen::genRangeCheck(GenTree* oper)
 {
-#ifdef FEATURE_SIMD
+#if defined(FEATURE_HW_INTRINSICS) && defined(FEATURE_SIMD)


Shouldn't the standalone GT_HWIntrinsic_CHK case also be handled (at the very least, based on the ifdefs elsewhere)?

So far, all the imm-intrinsics are SIMD. Do you think it is necessary?

I think that all #ifdefs concerning the HWIntrinsic Check should be consistent. So they either all need to be under #if defined(FEATURE_HW_INTRINSICS) && defined(FEATURE_SIMD) or they should all be under #ifdef FEATURE_HW_INTRINSICS. Having a mix of both (when there isn't two actual distinctions of where the code applies) just makes it confusing (and prone to breakage) later down the road.

Got it, I will add a standalone #ifdef GT_HWIntrinsic_CHK.

tannergooding · 2018-02-21T00:45:55Z

src/jit/emitxarch.cpp

@@ -5466,6 +5479,43 @@ void emitter::emitIns_SIMD_R_R_R(instruction ins, emitAttr attr, regNumber reg,
    }
 }

+static bool isSSEShift(instruction ins)


Is Sse the appropriate term here, since it also looks to cover AVX instructions, etc?

These special encoding shift instructions were introduced by SSE2, and AVX just gives them the VEX-encoding version. There are some AVX/AVX2-only shift instructions that don't belong to this group.

tannergooding · 2018-02-21T00:47:20Z

src/jit/emitxarch.cpp

+
+void emitter::emitIns_SIMD_R_R_I(instruction ins, emitAttr attr, regNumber reg, regNumber reg1, int ival)
+{
+    // TODO-XARCH refactoring emitIns_R_R_I to handle SSE2/AVX2 shift as well as emitIns_R_I


What about this isn't working today?

SSE2 shift instructions require special encoding, we need to improve emitIns_R_R_I to handle https://github.com/dotnet/coreclr/blob/master/src/jit/emitxarch.cpp#L3688-L3700 as well.

Could we log an actual bug, as well as this TODO?

Logged at https://github.com/dotnet/coreclr/issues/16543

fiigii · 2018-02-26T10:46:18Z

@CarolEidt Could you please take a look when you get a chance? This PR is blocking certain subsequent work.

CarolEidt

I am going to approve this PR so that it can be merged and unlbock other work (though I will defer to @tannergooding to do the merge. I hope that you will consider doing a quick follow-up PR to address my comment requests, and respond with clarification on whether there is a known case where you would have only one of HW_Flag_NoJmpTableIMM and HW_Flag_MaybeIMM set.
I filed #16568 to consider merging of the range check operators.

CarolEidt · 2018-02-26T13:09:20Z

src/jit/namedintrinsiclist.h

+
+    // NoJmpTable IMM
+    // the imm intrinsic does not need jumptable fallback when it gets non-const argument
+    HW_Flag_NoJmpTableIMM = 0x2000,


I still don't understand why both of these are required. In what case would we require a jump table if there was a non-immediate form available, and conversely, in what case would there be a non-immediate form available but we still require a jump table? Are there cases where the values covered by an immediate are somehow broader or otherwise disjoint from the values covered by a non-immediate form? I note that the existing intrinsics you added always have neither or both of these flags set.

I still don't understand why both of these are required.

@CarolEidt The new-style intrinsic is based on function names rather than signatures, and some intrinsics have "imm" and simple-SIMD overloads both. For example

public static Vector256<sbyte> Shuffle(Vector256<sbyte> value, Vector256<sbyte> mask); public static Vector256<int> Shuffle(Vector256<int> value, byte control) ;

so a category HW_Category_IMM is not enough to indicate a intrinsic requiring imm8 or not (the above intrinsic should have flag HW_Flag_MaybeIMM). Consequently, we have to use the function bool Compiler::isImmHWIntrinsic(NamedIntrinsic intrinsic, GenTree* lastOp).

In this kind of imm-intrinsic (HW_Flag_MaybeIMM), certain intrinsics are special, which have encoding that can carry the imm8 operand in XMM registers, for example

public static Vector256<ulong> ShiftRightLogical(Vector256<ulong> value, byte count); public static Vector256<ulong> ShiftRightLogical(Vector256<ulong> value, Vector128<ulong> count); ^^^^^^

So, this intrinsic can have flag HW_Flag_NoJmpTableIMM to use XMM version as the non-constant fallback (rather than a jump-table).

I note that the existing intrinsics you added always have neither or both of these flags set.

Consequently, Avx2.Shuffle will need HW_Flag_MaybeIMM but no HW_Flag_NoJmpTableIMM.

So, this intrinsic can have flag HW_Flag_NoJmpTableIMM to use XMM version as the non-constant fallback (rather than a jump-table).

I believe that the ISA design (carry the imm8 operand in XMM registers) is intentional for this situation, and ARM also has similar instructions.

OK, thanks for the explanation.

CarolEidt · 2018-02-26T13:41:11Z

src/jit/hwintrinsicxarch.cpp

+        return false;
+    }
+
+    if ((flagsOfHWIntrinsic(intrinsic) & HW_Flag_MaybeIMM) != 0 && genActualType(lastOp->TypeGet()) != TYP_INT)


It is not obvious why this is correct - it needs comments. This relies on the observation that all the HW_Flag_MaybeIMM cases have either an immmediate integer argument, or a non-integer argument, but not a non-constant integer argument.
Perhaps your "Return Value" comment should say:

Return true iff the intrinsics is an imm-intrinsic overload.
Note that some intrinsics, with HW_Flag_MaybeIMM set, have both imm (integer immediate) and vector (i.e. non-TYP_INT) overloads.

Thank you! Will do.

CarolEidt · 2018-02-26T13:44:19Z

src/jit/hwintrinsicxarch.cpp

+                return nullptr;
+            }
+
+            // TODO-XARCH-CQ repleace imm-intrinsic by certain lower-latency intrinsic based-on imm-values.


I would urge you to just remove this comment.

CarolEidt · 2018-02-26T13:49:17Z

src/jit/gtlist.h

@@ -97,6 +97,10 @@ GTNODE(SIMD_CHK         , GenTreeBoundsChk   ,0,GTK_SPECIAL|GTK_NOVALUE)// Compa
                                                                        // does the compare, so that it can be more easily optimized.  But that involves generating qmarks at import time...
 #endif // FEATURE_SIMD

+#ifdef FEATURE_HW_INTRINSICS
+GTNODE(HW_INTRINSIC_CHK  , GenTreeBoundsChk   ,0,GTK_SPECIAL|GTK_NOVALUE)// Compare whether an imm8 argument is in the valid range, and throw ArgumentOutOfRangeException if not.


I concede your point. These should probably be merged, though I am not 100% sure that is without risk, and I really don't want to hold this up for something that might lead to complications.

tannergooding · 2018-02-26T14:19:12Z

@fiigii, we should run the HWIntrinsic tests before merging.

NOTE: These jobs will currently fail due to an unrelated issue: https://github.com/dotnet/coreclr/issues/16566. I've held off on kicking these jobs myself until after the PR resolving that is merged: #16567

tannergooding · 2018-02-26T15:06:20Z

@fiigii, I would also like to see bugs logged for the 2-3 pending issues before merging (they all correspond to TODO comments added in this PR)

tannergooding · 2018-02-26T15:33:23Z

src/jit/emitxarch.cpp

+            emitIns_R_R(INS_movaps, attr, reg, reg1);
+        }
+
+        if ((ins == INS_psrldq || ins == INS_pslldq) && ival > 15)


Why just psrldq and pslldq instead of isSseShift

Oops, will fix.

tannergooding · 2018-02-26T15:36:58Z

src/jit/hwintrinsiccodegenxarch.cpp

+
+    const unsigned maxByte = (unsigned)Compiler::immUpperBoundOfHWIntrinsic(intrinsic) + 1;
+    assert(maxByte <= 256);
+    BasicBlock* jmpTable[256];


nit: BasicBlock* jmpTable[maxByte]?

maxByte is not a compile-time constant.

right, missed that 😄

tannergooding · 2018-02-26T15:38:44Z

src/jit/hwintrinsiccodegenxarch.cpp

+    for (unsigned i = 0; i < maxByte; i++)
+    {
+        jmpTable[i] = genCreateTempLabel();
+        JITDUMP("            DD      L_M%03u_BB%02u\n", Compiler::s_compMethodsCount, jmpTable[i]->bbNum);


nit: You removed JITDUMP("\n J_M%03u_DS%02u LABEL DWORD\n", Compiler::s_compMethodsCount, jmpTableBase); from above, but not this corresponding line.

@mikedn suggested removing that.

I think I suggested removing both :)

Yeah, I was indicating that if one line is removed, the other should be as well.

@mikedn @tannergooding Got it! Thanks.

tannergooding · 2018-02-26T15:40:24Z

src/jit/hwintrinsiccodegenxarch.cpp

+    for (unsigned i = 0; i < maxByte; i++)
+    {
+        genDefineTempLabel(jmpTable[i]);
+        emitSwCase((int)i);


Why not keep this as a byte?

The underlying functions all accept int, so it may not be necessary to introduce one more type to mess here...

But the loop variable is still unsigned :)

tannergooding · 2018-02-26T20:30:01Z

src/jit/codegenxarch.cpp

-    noway_assert(oper->OperGet() == GT_ARR_BOUNDS_CHECK);
-#endif // !FEATURE_SIMD
-
+    assert(oper->OperIsBoundsCheck());


Should this be kept as a noway_assert?

Not sure, but will fix.

No, noway_assert is pointless here. All other code gen functions use assert.

tannergooding · 2018-02-26T20:34:00Z

src/jit/hwintrinsicxarch.cpp

-        if (!lastOp->IsCnsIntOrI() && !mustExpand)
+        // The imm-HWintrinsics that do not accept all imm8 values may throw exception or transform to other
+        // intrinsics when the imm argument is not in the valid range
+        if ((flags & HW_Flag_FullRangeIMM) == 0)


Can you log a bug tracking us adding a helper for checking if a flag is set or unset?

Will do, and this comment should be updated, thanks.

And add a new range-check IR for x86 imm-intrinsics.

fiigii · 2018-02-26T21:05:31Z

src/jit/hwintrinsiccodegenxarch.cpp

+    for (unsigned i = 0; i < maxByte; i++)
+    {
+        genDefineTempLabel(jmpTable[i]);
+        emitSwCase(i);


@tannergooding @mikedn Changed emitSwCase to accept unsigned, so that this function only uses unsigned variables.

tannergooding · 2018-02-26T21:09:50Z

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

fiigii · 2018-02-26T21:12:11Z

@CarolEidt @tannergooding @mikedn Thank you so much for your reviwe. All the feedback should already get fixed. Logged the remaining works at https://github.com/dotnet/coreclr/issues/16594 https://github.com/dotnet/coreclr/issues/16575 https://github.com/dotnet/coreclr/issues/16543 https://github.com/dotnet/coreclr/issues/16568

tannergooding · 2018-02-26T21:25:38Z

@fiigii thanks for responding to the feedback so far.

I'll get this merged after all the tests complete (and pass).

CarolEidt · 2018-02-26T21:28:27Z

Thanks for the changes & responses. I'm happy with where we are on this.

fiigii · 2018-02-26T21:45:00Z

@4creators This PR is going to be merged. Would you like to complete the SSE2 intrinsic implementation? As I know, the code freeze date of .NET Core 2.1 may be coming soon, we probably prefer to enable all the SSE/SSE2/SSE3/SSSE3/SSE4.1/AVX intrinsics in this version of runtime.
Do you have any plan of SSE2 intrinsic? If you have any problem that cannot implement all of them in a short time, please just let me know, we can cooperate to speed up the process.

4creators · 2018-02-26T22:49:51Z

@fiigii Thanks for information. I will try to implement remaining SSE2 intrinsics asap. What is a plan on AVX2? Do we intend to finish them for 2.1 release?

fiigii · 2018-02-26T22:59:07Z

What is a plan on AVX2? Do we intend to finish them for 2.1 release?

I will try my best to implement as many as AVX2 intrinsics (depends on the deadline).

tannergooding · 2018-02-26T23:03:54Z

@4creators, if you could send me an e-mail (tagoo at microsoft dot com) it might be a bit easier to sync up on this without polluting the thread.

4creators · 2018-02-26T23:07:36Z

if you could send me an e-mail (tagoo at microsoft dot com)

@tannergooding Just did it

fiigii · 2018-02-27T01:31:02Z

OSX tests get build timed out. Other tests all passed.

4creators reviewed Feb 2, 2018

View reviewed changes

fiigii changed the title ~~Update the table-driven framework to support x86 imm-intrinsics~~ [WIP] Update the table-driven framework to support x86 imm-intrinsics Feb 2, 2018

jkotas added the area-CodeGen label Feb 3, 2018

mikedn reviewed Feb 3, 2018

View reviewed changes

tannergooding reviewed Feb 6, 2018

View reviewed changes

4creators reviewed Feb 6, 2018

View reviewed changes

fiigii force-pushed the refactor-imm branch 5 times, most recently from 1ae1ee1 to b847cb0 Compare February 8, 2018 00:32

4creators reviewed Feb 8, 2018

View reviewed changes

fiigii commented Feb 8, 2018

View reviewed changes

fiigii force-pushed the refactor-imm branch 2 times, most recently from b157c86 to f84095e Compare February 20, 2018 23:35

fiigii changed the title ~~[WIP] Update the table-driven framework to support x86 imm-intrinsics~~ Update the table-driven framework to support x86 imm-intrinsics Feb 20, 2018

fiigii force-pushed the refactor-imm branch from f84095e to 9658b59 Compare February 21, 2018 00:40

tannergooding reviewed Feb 21, 2018

View reviewed changes

fiigii force-pushed the refactor-imm branch from b6f43f1 to e89bf17 Compare February 25, 2018 18:26

CarolEidt approved these changes Feb 26, 2018

View reviewed changes

tannergooding reviewed Feb 26, 2018

View reviewed changes

fiigii force-pushed the refactor-imm branch from e89bf17 to 0749831 Compare February 26, 2018 20:20

tannergooding reviewed Feb 26, 2018

View reviewed changes

Update the table-driven framework to support x86 imm-intrinsics.

7a24967

And add a new range-check IR for x86 imm-intrinsics.

fiigii force-pushed the refactor-imm branch from 0749831 to 7a24967 Compare February 26, 2018 21:02

fiigii commented Feb 26, 2018

View reviewed changes

tannergooding merged commit e61f7f4 into dotnet:master Feb 27, 2018

4creators mentioned this pull request Jan 31, 2020

Lack of coordination and overlapping work while implementing Intel hardware intrinsics dotnet/runtime#9656

Closed


		// Emit the jump table

		JITDUMP("\n J_M%03u_DS%02u LABEL DWORD\n", Compiler::s_compMethodsCount, jmpTableBase);

Update the table-driven framework to support x86 imm-intrinsics #16183

Update the table-driven framework to support x86 imm-intrinsics #16183

Uh oh!

Conversation

fiigii commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fiigii commented Feb 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fiigii Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fiigii commented Feb 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fiigii commented Feb 2, 2018 •

edited

Loading

fiigii Feb 8, 2018 •

edited

Loading

fiigii Feb 26, 2018 •

edited

Loading