Implement AVX2 Gather intrinsic #19392

fiigii · 2018-08-09T22:01:25Z

Contribute to #16653

fiigii · 2018-08-09T22:01:55Z

src/System.Private.CoreLib/shared/System/Runtime/Intrinsics/X86/Avx2.PlatformNotSupported.cs

@@ -628,12 +628,12 @@ public abstract class Avx2 : Avx
        public static unsafe Vector256<double> GatherVector256(double* baseAddress, Vector128<int> index, byte scale) { throw new PlatformNotSupportedException(); }
        /// <summary>
        /// __m128i _mm256_i64gather_epi32 (int const* base_addr, __m256i vindex, const int scale)
-        ///   VPGATHERQD ymm, vm64y, ymm
+        ///   VPGATHERQD xmm, vm64y, xmm


Fixed some doc mistake of Gather intrinsic

fiigii · 2018-08-09T22:05:16Z

src/System.Private.CoreLib/shared/System/Runtime/Intrinsics/X86/Avx2.cs

+                case 8:
+                    return GatherVector128(baseAddress, index, 8);
+                default:
+                    throw new ArgumentOutOfRangeException();


The scale parameter has discrete valid values, so the RyuJIT bound-check does not work. Use managed non-const fallback to handle Gather intrinsic, which we do not need to worry about compiler optimization on the switch table.

You should add comments to that effect in the function header for this method.

Added a comment "The scale parameter should be 1, 2, 4 or 8, otherwise, ArgumentOutOfRangeException will be thrown." for each function header.

fiigii · 2018-08-09T22:08:20Z

src/jit/gentree.h

@@ -4112,6 +4112,7 @@ struct GenTreeSIMD : public GenTreeJitIntrinsic
 struct GenTreeHWIntrinsic : public GenTreeJitIntrinsic
 {
    NamedIntrinsic gtHWIntrinsicId;
+    var_types gtIndexBaseType; // for AVX2 Gather* intrinsics


Gather intrinsics have complex overloads that need additional information (the base-type of index vector) for codegen, so adding a field in IR. But that let GenTreeHWIntrinsic become a large node. @CarolEidt do you think it ok?

fiigii · 2018-08-09T22:10:18Z

src/jit/hwintrinsicxarch.cpp

-#endif
-
-        return 3;
+        return numArgs;


GatherMaskVector* have 5 parameters that breaks our previous numArgs assumption.

Do we actually have to carry all 5 args through? They eventually get folded down to a target register, vsib encoding, and mask register, so I would think we we could do some of this folding in the importer.

so I would think we we could do some of this folding in the importer.

That seems to require a new IR to contain the folded VSIB.

Yeah, I guess we could only fold it to 3 if we knew the register for index before-hand....

It just seems bad to make this many changes for a single family of instructions, since it will impact the 90% case (which is the other HWIntrinsics)...

I think this impact should be okay, as we have had this code (extracting numArgs from IR).

many changes for a single family of instructions

We may have more intrinsics with >3 arguments, like SetVector* that we need to bring its IR through for all-const optimization.

tannergooding · 2018-08-09T22:34:43Z

This section is important and we should ensure it doesn't impact any existing assumptions:

fiigii · 2018-08-09T22:46:10Z

This section is important and we should ensure it doesn't impact any existing assumptions:

Yes, I have addressed all the special encoding requests. But some fault issues (like the last one in the above list) are difficult to test...

4creators · 2018-08-09T23:33:35Z

src/System.Private.CoreLib/shared/System/Runtime/Intrinsics/X86/Avx2.cs

@@ -546,243 +546,963 @@ public abstract class Avx2 : Avx
        /// __m128i _mm_i32gather_epi32 (int const* base_addr, __m128i vindex, const int scale)
        ///   VPGATHERDD xmm, vm32x, xmm
        /// </summary>
-        public static unsafe Vector128<int> GatherVector128(int* baseAddress, Vector128<int> index, byte scale) => GatherVector128(baseAddress, index, scale);
+        public static unsafe Vector128<int> GatherVector128(int* baseAddress, Vector128<int> index, byte scale)
+        {


Perhaps we should apply AggressiveInlining attribute to all managed implementations

No, the jump-table fallback should always be a call-node.

4creators · 2018-08-09T23:37:01Z

src/jit/emitxarch.cpp

+                             regNumber   index,
+                             int         scale,
+                             int         offs)
+{


Coding convention: method needs header

4creators · 2018-08-09T23:37:15Z

src/jit/emitxarch.cpp

@@ -4326,6 +4330,53 @@ void emitter::emitIns_R_R_AR(instruction ins, emitAttr attr, regNumber reg1, reg
    emitCurIGsize += sz;
 }

+bool IsAVX2GatherInstruction(instruction ins)


Coding convention: method needs header

fiigii · 2018-08-10T18:00:13Z

src/jit/gentree.h

@@ -4112,6 +4112,7 @@ struct GenTreeSIMD : public GenTreeJitIntrinsic
 struct GenTreeHWIntrinsic : public GenTreeJitIntrinsic
 {
    NamedIntrinsic gtHWIntrinsicId;
+    var_types      gtIndexBaseType; // for AVX2 Gather* intrinsics


Gather intrinsics have complex overloads that need additional information (the base-type of index vector) for codegen, so adding a field in IR. But that let GenTreeHWIntrinsic become a large node. @CarolEidt do you think it ok?

It is probably not a big issue for intrinsics, though for methods with heavy intrinsic usage it could be an impact. Did you consider deriving from GenTreeHWIntrinsic to make a specialized node?

It might also be worth considering whether to define something like GenTreeHWIntrinsicBig, to isolate both the additional fields as well as the extra operands.

Perhaps make NamedIntrinsic unsigned short? 64k intrinsics ought to be enough for anybody.

There are also spare bytes in the GenTree class but I don't know an elegant way to use those in derived classes.

Perhaps make NamedIntrinsic unsigned short? 64k intrinsics ought to be enough for anybody.

@mikedn Good point! Yes, 64k is definitely enough for the foreseeable future (AVX-512 based ISAs).

make NamedIntrinsic unsigned short

Made this change, now GenTreeHWIntrinsic is still a small node.

What is the gtIndexBaseType actually needed for? Can it not be inferred from the rest of the signature?

CarolEidt

I didn't review the implementation in System.Private.CoreLib.dll, and I will be out of the office until next Thursday. But I had a few comments.

CarolEidt · 2018-08-11T00:42:11Z

src/jit/emitxarch.cpp

@@ -4326,6 +4330,67 @@ void emitter::emitIns_R_R_AR(instruction ins, emitAttr attr, regNumber reg1, reg
    emitCurIGsize += sz;
 }

+// return true if ins is an AVX2 Gather instruction


Since this is new, you should add a header here as well.

Will do, thanks.

CarolEidt · 2018-08-11T00:42:51Z

src/jit/emitxarch.h

+                    regNumber   base,
+                    regNumber   index,
+                    int         scale,
+                    int         offs);


I don't know if this was formatted this way by jit-format, but I'd prefer to see it declared in a format more similar to those around it.

Yes, this was given by jit-format, let me try to unify the format.

This format is given by clang-format and cannot manually change...

CarolEidt · 2018-08-11T00:43:44Z

src/jit/gentree.cpp

+     * isn't handling self-assignment of struct variables correctly.  This issue may not
+     * surface if struct promotion is ON (which is the case on x86/arm).  But still the
+     * fundamental issue exists that needs to be addressed.
+     */


Why was this changed?

CarolEidt · 2018-08-11T00:45:01Z

src/jit/gentree.h

-        INDEBUG(gtCostsInitialized =
-                    tree->gtCostsInitialized;) // If the 'tree' costs aren't initialized, we'll hit an assert below.
+        INDEBUG(gtCostsInitialized = tree->gtCostsInitialized;) // If the 'tree' costs aren't initialized, we'll hit an
+                                                                // assert below.


Again, not sure why this changed - but if it's going to change, the comment should be moved above.

Hmm, on my computer, jit-format + vs code is not stable on a few jit source files, which can output different format with each run, especially for some comments or whitespace.
Will fix, sorry for the mess.

CarolEidt · 2018-08-11T00:47:33Z

src/jit/gentree.h

@@ -4112,6 +4112,7 @@ struct GenTreeSIMD : public GenTreeJitIntrinsic
 struct GenTreeHWIntrinsic : public GenTreeJitIntrinsic
 {
    NamedIntrinsic gtHWIntrinsicId;
+    var_types      gtIndexBaseType; // for AVX2 Gather* intrinsics


It is probably not a big issue for intrinsics, though for methods with heavy intrinsic usage it could be an impact. Did you consider deriving from GenTreeHWIntrinsic to make a specialized node?

CarolEidt · 2018-08-11T00:48:43Z

src/jit/hwintrinsiccodegenxarch.cpp

+                addrIndexReg = op3Reg;
+                indexOp      = op3;
+
+                // the mask register will be cleaned by gather instructions


will be cleared by ...?

Oops, will fix.

CarolEidt · 2018-08-11T00:49:37Z

src/jit/hwintrinsiccodegenxarch.cpp

@@ -1177,6 +1177,8 @@ void CodeGen::genHWIntrinsicJumpTableFallback(NamedIntrinsic            intrinsi
                                              HWIntrinsicSwitchCaseBody emitSwCase)
 {
    assert(nonConstImmReg != REG_NA);
+    // AVX2 Gather intrinsics use managed non-const fallback


This deserves more explanation.

Thanks, will add more comments.

CarolEidt · 2018-08-11T00:53:08Z

src/jit/lowerxarch.cpp

+            MakeSrcContained(node, lastOp);
+        }
+    }
+
    if (!HWIntrinsicInfo::SupportsContainment(intrinsicId))
    {
        // Exit early if containment isn't supported


You're no longer really exiting early - was there some reason you moved the immediate containment above this?

Before this PR, IMM intrinsics do not have NoContainment ones. But AVX2 Gather* are IMM intrinsic AND memory-load intrinsic that cannot work with containment (so they have NoContainment flag).

So we need to moving the immediate containment above the early exiting, which marks the IMM argument of Gather before return.

So then I think this comment needs to be deleted.

CarolEidt · 2018-08-11T00:55:29Z

src/jit/gentree.h

@@ -4112,6 +4112,7 @@ struct GenTreeSIMD : public GenTreeJitIntrinsic
 struct GenTreeHWIntrinsic : public GenTreeJitIntrinsic
 {
    NamedIntrinsic gtHWIntrinsicId;
+    var_types      gtIndexBaseType; // for AVX2 Gather* intrinsics


It might also be worth considering whether to define something like GenTreeHWIntrinsicBig, to isolate both the additional fields as well as the extra operands.

fiigii · 2018-08-13T19:45:57Z

@dotnet-bot test Windows_NT x64 Checked jitincompletehwintrinsic
@dotnet-bot test Windows_NT x64 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Windows_NT x64 Checked jitx86hwintrinsicnoavx2
@dotnet-bot test Windows_NT x64 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Windows_NT x64 Checked jitnox86hwintrinsic

@dotnet-bot test Windows_NT x86 Checked jitincompletehwintrinsic
@dotnet-bot test Windows_NT x86 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Windows_NT x86 Checked jitx86hwintrinsicnoavx2
@dotnet-bot test Windows_NT x86 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Windows_NT x86 Checked jitnox86hwintrinsic

@dotnet-bot test Ubuntu x64 Checked jitincompletehwintrinsic
@dotnet-bot test Ubuntu x64 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Ubuntu x64 Checked jitx86hwintrinsicnoavx2
@dotnet-bot test Ubuntu x64 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Ubuntu x64 Checked jitnox86hwintrinsic

fiigii · 2018-08-20T17:37:52Z

Addressed feedback. @tannergooding @CarolEidt @eerhardt could you please take a look? Some applications dotnet/machinelearning#691 need the Gather intrinsic.

tannergooding · 2018-08-20T23:19:43Z

I will be able to review Wednesday (or possibly late tomorrow), after I get back from vacation.

fiigii · 2018-08-23T18:16:40Z

@tannergooding @CarolEidt ping?

CarolEidt

Sorry for the delay in reviewing.
I have a number of questions, and suggestions for additional commenting.

CarolEidt · 2018-08-23T20:39:31Z

src/jit/gentree.cpp

@@ -362,7 +362,7 @@ void GenTree::InitNodeSize()
 #endif // FEATURE_SIMD

 #ifdef FEATURE_HW_INTRINSICS
-    static_assert_no_msg(sizeof(GenTreeHWIntrinsic)  <= TREE_NODE_SZ_SMALL);
+    static_assert_no_msg(sizeof(GenTreeHWIntrinsic)  <= TREE_NODE_SZ_SMALL); // *** large node


It appears that the 'large node' comment is no longer needed.

CarolEidt · 2018-08-23T20:41:31Z

src/jit/hwintrinsicxarch.cpp

@@ -532,7 +592,8 @@ GenTree* Compiler::addRangeCheckIfNeeded(NamedIntrinsic intrinsic, GenTree* last
    assert(lastOp != nullptr);
    // Full-range imm-intrinsics do not need the range-check
    // because the imm-parameter of the intrinsic method is a byte.


This comment should mention the gather case.

CarolEidt · 2018-08-23T20:42:40Z

src/System.Private.CoreLib/shared/System/Runtime/Intrinsics/X86/Avx2.cs

+                case 8:
+                    return GatherVector128(baseAddress, index, 8);
+                default:
+                    throw new ArgumentOutOfRangeException();


You should add comments to that effect in the function header for this method.

CarolEidt · 2018-08-23T21:36:24Z

src/jit/hwintrinsiccodegenxarch.cpp

+                addrIndexReg = op3Reg;
+                indexOp      = op3;
+
+                // the mask register will be cleared by gather instructions


I'm not sure I understand the point of this comment. The maskReg is a tempreg, so I don't see that it matters whether it will be cleared. Perhaps you mean that we need to copy op4 to a temp reg to use for the mask because it will be over-written?

Perhaps you mean that we need to copy op4 to a temp reg to use for the mask because it will be over-written?

Yes, will update the comment.

CarolEidt · 2018-08-23T21:43:12Z

src/jit/hwintrinsiccodegenxarch.cpp

+                if (targetReg != op1Reg)
+                {
+                    // copy source vector to the target register for masking merge
+                    emit->emitIns_R_R(INS_movaps, attr, targetReg, op1Reg);


What if op2Reg == targetReg? I don't think it's been set as delayFree.

Op2Reg is a GPR (base register of the address) and cannot be same as the vector target register.

Right - thanks for clarifying.

A comment clarifying this would be nice

CarolEidt · 2018-08-23T21:43:14Z

src/jit/hwintrinsiccodegenxarch.cpp

+            }
+
+            regNumber lastOpReg = lastOp->gtRegNum;
+            genConsumeRegs(lastOp);


What is the purpose of having this here, rather than always consuming op3 before the if-stmt above, and then taking care of the other sources in that if stmt?

CarolEidt · 2018-08-23T21:45:18Z

src/jit/lowerxarch.cpp

+            MakeSrcContained(node, lastOp);
+        }
+    }
+
    if (!HWIntrinsicInfo::SupportsContainment(intrinsicId))
    {
        // Exit early if containment isn't supported


So then I think this comment needs to be deleted.

CarolEidt · 2018-08-23T21:46:51Z

src/jit/lsraxarch.cpp

+            case NI_AVX2_GatherMaskVector256:
+            {
+                assert(numArgs == 5);
+                // Any pair of the index, mask, or destination registers should be different


This comment mentions index, mask and destination; it needs to map those to the opN names. Also, what about the address? Doesn't that also need to be distinct from the target, as the target will be overwritten?

Thanks, will update the comment.

Also, what about the address? Doesn't that also need to be distinct from the target, as the target will be overwritten?

Don't you mean the address base register? If yes, that is a GPR and cannot be same as the vector target register.

fiigii · 2018-08-23T23:39:17Z

@CarolEidt Thank you for the review and suggestions, I have addressed all the feedback.

CarolEidt · 2018-08-24T23:34:02Z

@tannergooding - did you want to review this before merging?

fiigii · 2018-08-27T17:04:30Z

@tannergooding ping?

tannergooding · 2018-08-27T17:06:49Z

src/System.Private.CoreLib/shared/System/Runtime/Intrinsics/X86/Avx2.cs

+                case 8:
+                    return GatherVector128(baseAddress, index, 8);
+                default:
+                    throw new ArgumentOutOfRangeException();


nit: Should be throw new ArgumentOutOfRangeException(nameof(scale))

(same elsewhere as well)

Other intrinsic throw the exception without the argument name. Shall we keep consistent?

No, we should fix the other intrinsics to be correct instead 😄

Agree, logged at https://github.com/dotnet/coreclr/issues/19689

tannergooding · 2018-08-27T17:12:51Z

src/jit/emitxarch.cpp

+//    scale - the scale number of VSIB
+//    offs - the offset added to the memory address from base
+//
+void emitter::emitIns_R_AR_R(instruction ins,


I wonder if we shouldn't be encoding the scale/offs in an Indir node and letting emitHandleMemOp handle it

@CarolEidt ?

The scale/offs would be on a GenTreeAddrMode - presumably a GT_LEA. It would make sense for that to be a child of the gather node, which would not only consolidate the scale/offs handling, but also would reduce the operand count of the gather.

@CarolEidt Thanks for the comments. Do you suggest to make the change in this PR? Or I can improve it in a new PR?

I think it would be fine (and probably a bit cleaner) to do it as a separate PR.

tannergooding · 2018-08-27T17:15:44Z

src/jit/gentree.cpp

@@ -17522,6 +17522,11 @@ bool GenTreeHWIntrinsic::OperIsMemoryLoad()
    {
        // Some AVX instructions here also have MemoryLoad sematics

+        if (HWIntrinsicInfo::isAVX2GatherIntrinsic(gtHWIntrinsicId))


Why was this inserted here, rather than as part of the rest of the logic flow below?

Because Gather has 3-op and 5-op overloads, this early check simplified the code.

The check would have remained correct and simple by changing the code as such:

- // Do we have 3 operands? - if (HWIntrinsicInfo::lookupNumArgs(this) != 3) + // Do we have less than 3 operands? + if (HWIntrinsicInfo::lookupNumArgs(this) < 3)

This also ensures that we aren't adding a "fast-path" for checks that don't need it

Good point, will do.

tannergooding · 2018-08-27T17:17:07Z

src/jit/gentree.h

@@ -479,8 +479,8 @@ struct GenTree
    // happening.
    void CopyCosts(const GenTree* const tree)
    {
-        INDEBUG(gtCostsInitialized =
-                    tree->gtCostsInitialized;) // If the 'tree' costs aren't initialized, we'll hit an assert below.
+        // If the 'tree' costs aren't initialized, we'll hit an assert below.


Why are all these random comment re-alignments popping up? They seem unrelated to the PR.

Clang-format is confused by some comments, so I fixed here to avoid more unrelated changes from clang-format.

tannergooding · 2018-08-27T17:18:54Z

src/jit/hwintrinsiccodegenxarch.cpp

+        case NI_AVX2_GatherMaskVector128:
+        case NI_AVX2_GatherMaskVector256:
+        {
+            GenTreeArgList* list = op1->AsArgList();


nit: It is helpful to break up the argList blocks, for readability: Ex:

coreclr/src/jit/hwintrinsiccodegenxarch.cpp

Line 217 in 7f36c93

GenTreeArgList* argList = op1->AsArgList();

tannergooding · 2018-08-27T17:20:11Z

src/jit/hwintrinsiccodegenxarch.cpp

+            genConsumeRegs(op2);
+            genConsumeRegs(op3);
+
+            if (intrinsicId == NI_AVX2_GatherMaskVector128 || intrinsicId == NI_AVX2_GatherMaskVector256)


Why not just differentiate on the argCount?

I thought intrinsic id is more readable, as other developers do not need to check API files.

I would think a comment or an assert would be sufficient for that

Ok, will do.

tannergooding · 2018-08-27T17:30:29Z

src/jit/hwintrinsicxarch.cpp

-    if (mustExpand && !HWIntrinsicInfo::HasFullRangeImm(intrinsic) && HWIntrinsicInfo::isImmOp(intrinsic, lastOp))
+    // AVX2 Gather intrinsics no not need the range-check
+    // because their imm-parameter have discrete valid values that are handle by managed code
+    if (mustExpand && !HWIntrinsicInfo::isAVX2GatherIntrinsic(intrinsic) &&


It seems like this should be the last check in the && chain, since the other two are more likely to exit the chain first

Agree, will fix

tannergooding · 2018-08-27T17:32:08Z

src/jit/lowerxarch.cpp

@@ -2576,9 +2576,19 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
    GenTree* op2 = node->gtGetOp2();
    GenTree* op3 = nullptr;

+    if (HWIntrinsicInfo::lookupCategory(intrinsicId) == HW_Category_IMM)


Why was this moved up?

Before this PR, IMM intrinsics do not have NoContainment ones. But AVX2 Gather* are IMM intrinsic AND memory-load intrinsic that cannot work with containment (so they have NoContainment flag).

So we need to move the immediate containment above the early exiting, which marks the IMM argument of Gather before return.

Still not sure I see the problem.

There are multiple intrinsics which can be sometimes contained, or partially contained (just the immediate node). You don't mark those as NoContainment, but exit below (as a special case) instead.

tannergooding · 2018-08-27T17:33:45Z

tests/src/JIT/HardwareIntrinsics/X86/Avx2/GatherMaskVector128_r.csproj

@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="utf-8"?>


Please add these as templated tests, since we are trying to reduce the number of manually managed tests that we have for the intrinsics.

and when doing so, please ensure that the new templates follow the same general pattern as previous templates, to ensure that all the special-cases are covered (such as indirect invocation)

Gather intrinsic have very special overloads and semantics that do not work with the current test template framework.

Let me move the test cases to template later in a new PR, some applications are waiting for these intrinsic

Then, at the very least, you need to ensure all the special cases are still covered in the manual code.

The current templates cover 14 or so scenarios that are meant to test the various code patterns that must be supported and that might influence the JIT (including things like indirect invocation, various patterns that may impact containment, etc).

I'm also not sure I agree with "does not work with the test template framework".

The framework is very generic and just works with a template and named string values. You should be able to create a template that fits your needs (and that follows the existing pattern) and have it just work.

fiigii · 2018-08-27T21:00:43Z

@tannergooding @CarolEidt Thank you for the review. Addressed all the feedback.

Will move the tests to template and investigate the indir solution later.

tannergooding · 2018-08-27T21:07:21Z

I would still like to see test coverage for the indirect invocation case, to validate that it is working as intended.

fiigii · 2018-08-31T23:43:07Z

@dotnet-bot test Windows_NT x64 Checked jitincompletehwintrinsic
@dotnet-bot test Windows_NT x64 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Windows_NT x64 Checked jitx86hwintrinsicnoavx2
@dotnet-bot test Windows_NT x64 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Windows_NT x64 Checked jitnox86hwintrinsic

@dotnet-bot test Windows_NT x86 Checked jitincompletehwintrinsic
@dotnet-bot test Windows_NT x86 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Windows_NT x86 Checked jitx86hwintrinsicnoavx2
@dotnet-bot test Windows_NT x86 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Windows_NT x86 Checked jitnox86hwintrinsic

@dotnet-bot test Ubuntu x64 Checked jitincompletehwintrinsic
@dotnet-bot test Ubuntu x64 Checked jitx86hwintrinsicnoavx
@dotnet-bot test Ubuntu x64 Checked jitx86hwintrinsicnoavx2
@dotnet-bot test Ubuntu x64 Checked jitx86hwintrinsicnosimd
@dotnet-bot test Ubuntu x64 Checked jitnox86hwintrinsic

fiigii · 2018-09-04T18:14:38Z

I would still like to see test coverage for the indirect invocation case,

@tannergooding I have added indirect invocation cases, can we merge this PR?

tannergooding · 2018-09-04T21:43:59Z

I'm fine with merging. I would appreciate it if you could log a bug tracking the test code cleanup (porting it to a template, etc).

@CarolEidt, any other feedback on your end?

fiigii · 2018-09-04T21:51:06Z

@tannergooding thanks, will do.

fiigii · 2018-09-04T22:04:23Z

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

CarolEidt · 2018-09-04T22:15:09Z

No additional feedback - I'm happy with merging

tannergooding · 2018-09-04T22:17:28Z

Will merge after the last test finishes (it had timed out previously, so I restarted it).

fiigii · 2018-09-04T22:18:58Z

@tannergooding @CarolEidt Thank you so much for the review!

fiigii · 2018-09-05T19:55:08Z

@tannergooding could you please merge this PR?

tannergooding · 2018-09-05T19:56:46Z

Done. Thanks for the work @fiigii

fiigii commented Aug 9, 2018

View reviewed changes

4creators reviewed Aug 9, 2018

View reviewed changes

fiigii force-pushed the gather branch from 3dba8aa to 48e9338 Compare August 10, 2018 17:59

fiigii commented Aug 10, 2018

View reviewed changes

fiigii force-pushed the gather branch from 48e9338 to 3fc883b Compare August 10, 2018 18:23

CarolEidt reviewed Aug 11, 2018

View reviewed changes

fiigii force-pushed the gather branch from 3fc883b to d55cda2 Compare August 13, 2018 19:23

helloguo mentioned this pull request Aug 20, 2018

Port all relevant AVX hardware intrinsics C# APIs from SIMD native algorithms dotnet/machinelearning#691

Merged

fiigii force-pushed the gather branch from d55cda2 to 8de1528 Compare August 23, 2018 00:13

CarolEidt reviewed Aug 23, 2018

View reviewed changes

fiigii force-pushed the gather branch from 8de1528 to 7f36c93 Compare August 23, 2018 23:34

CarolEidt approved these changes Aug 24, 2018

View reviewed changes

tannergooding reviewed Aug 27, 2018

View reviewed changes

fiigii force-pushed the gather branch from 7f36c93 to 2e1ecd0 Compare August 27, 2018 20:58

fiigii force-pushed the gather branch 2 times, most recently from 09f75c8 to 74236e5 Compare August 27, 2018 22:15

Add managed non-const fall-back for AVX2 gather intrinsic

dde6be2

fiigii force-pushed the gather branch from 74236e5 to ea92a6d Compare August 31, 2018 21:18

FeiPengIntel added 2 commits August 31, 2018 16:42

Add test for AVX2 gather intrinsic

c0510fb

Implement AVX2 Gather intrinsic in JIT

1cef300

fiigii force-pushed the gather branch from ea92a6d to 1cef300 Compare August 31, 2018 23:42

tannergooding approved these changes Sep 4, 2018

View reviewed changes

tannergooding merged commit 0f6597f into dotnet:master Sep 5, 2018

eerhardt mentioned this pull request Oct 9, 2018

Remove the AddScaleSU performance regression for AVX dotnet/machinelearning#1195

Open

fiigii mentioned this pull request Jan 31, 2020

Hardware intrinsic throw ArgumentOutOfRangeException with IMM parameter name dotnet/runtime#10982

Open

Implement AVX2 Gather intrinsic #19392

Implement AVX2 Gather intrinsic #19392

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fiigii Aug 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Aug 9, 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!

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Aug 9, 2018

Uh oh!

fiigii commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fiigii Aug 9, 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!

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!

CarolEidt left a comment

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 commented Aug 9, 2018 •

edited

Loading

fiigii Aug 9, 2018 •

edited

Loading

tannergooding Aug 9, 2018 •

edited

Loading

fiigii Aug 9, 2018 •

edited

Loading

fiigii commented Aug 20, 2018 •

edited

Loading