Add JIT intrinsics support for vector conversion on AMD64 and x86 #10662

helloguo · 2017-04-03T17:37:32Z

This PR adds JIT intrinsic support for vector conversion/narrow/widen on AMD64 SSE2, SSE34 and AVX. It is built on #9920 and #9318. The intrinsics will be tested by #10467.

The following APIs are accelerated by JIT intrinsics provided in this PR:

public static partial class Vector
{
    public static void Widen(Vector<byte> source, out Vector<ushort> dest1, out Vector<ushort> dest2);
    public static void Widen(Vector<ushort> source, out Vector<uint> dest1, out Vector<uint> dest2);
    public static void Widen(Vector<uint> source, out Vector<ulong> dest1, out Vector<ulong> dest2);
    public static void Widen(Vector<sbyte> source, out Vector<short> dest1, out Vector<short> dest2);
    public static void Widen(Vector<short> source, out Vector<int> dest1, out Vector<int> dest2);
    public static void Widen(Vector<int> source, out Vector<long> dest1, out Vector<long> dest2);
    public static void Widen(Vector<float> source, out Vector<double> dest1, out Vector<double> dest2);

    public static Vector<byte> Narrow(Vector<ushort> source1, Vector<ushort> source2);
    public static Vector<ushort> Narrow(Vector<uint> source1, Vector<uint> source2);
    public static Vector<uint> Narrow(Vector<ulong> source1, Vector<ulong> source2);
    public static Vector<sbyte> Narrow(Vector<short> source1, Vector<short> source2);
    public static Vector<short> Narrow(Vector<int> source1, Vector<int> source2);
    public static Vector<int> Narrow(Vector<long> source1, Vector<long> source2);
    public static Vector<float> Narrow(Vector<double> source1, Vector<double> source2);

    public static Vector<float> ConvertToSingle(Vector<int> value);
    public static Vector<float> ConvertToSingle(Vector<uint> value);
    public static Vector<double> ConvertToDouble(Vector<long> value);
    public static Vector<double> ConvertToDouble(Vector<ulong> value);
    public static Vector<int> ConvertToInt32(Vector<float> value);
    public static Vector<uint> ConvertToUInt32(Vector<float> value);
    public static Vector<long> ConvertToInt64(Vector<double> value);
    public static Vector<ulong> ConvertToUInt64(Vector<double> value);
}

The semantics of above APIs can be found at https://github.com/dotnet/corefx/issues/15957. The C# implementation of above APIs can be found at dotnet/corefx#16276.

Fix https://github.com/dotnet/coreclr/issues/9317.

helloguo · 2017-04-03T17:55:45Z

@BruceForstall @russellhadley @CarolEidt PTAL

helloguo · 2017-04-03T21:12:48Z

@dotnet-bot test OSX10.12 x64 Checked Build and Test
@dotnet-bot test Ubuntu x64 Checked Build and Test

BruceForstall · 2017-04-04T21:20:10Z

src/jit/emitxarch.cpp

@@ -214,7 +217,8 @@ bool TakesRexWPrefix(instruction ins, emitAttr attr)
    // cased here.
    //
    // Rex_jmp = jmp with rex prefix always requires rex.w prefix.
-    if (ins == INS_movsx || ins == INS_rex_jmp)
+    // vpermq requires w bit to set to 1
+    if (ins == INS_movsx || ins == INS_rex_jmp || ins == INS_vpermq)
    {


I don't think this is correct. vpermq is encoded with a VEX prefix (with w set to 1), not with a REX.W prefix. And this function is only asking about REX.W.

Thanks for pointing it out. Yes, vpermq needs VEX w1 bit. How about creating two functions named TakesVexWPrefix() and AddVexWPrefix() to handle VEX?

We already have TakesVexPrefix() and AddVexPrefix(). Maybe what you want is a function that returns true if the VEX prefix W bit needs to be set (I assume nobody required this until now). Perhaps that should be called RequiresVexPrefixWBit(instruction ins)? Then, AddVexPrefix() could have code like:

if (RequiresVexPrefixWBit(ins)) { code |= ... <magic W bit mask> ... }

This doesn't seem right to me. The VEX.W prefix, I believe, is generally the AVX encoding of the corresponding REX.W prefix. We do have encodings that use the VEX.W prefix, and it is set based on the opcode size. I think it may be that the vpermq encoding needs to have the correct operand size, in which case I believe that the W bit would be set appropriately.

In reply to: 109974462 [](ancestors = 109974462)

The VEX.W prefix, I believe, is generally the AVX encoding of the corresponding REX.W prefix.

I agree. From Intel® 64 and IA-32 Architectures Software Developer’s Manual section 2.3.5, "Three-byte form of the VEX prefix provides the functionality of REX.W only to specific instructions that need to override default 32-bit operand size for a general purpose register to 64-bit size in 64-bit mode. For those applicable instructions, VEX.W field provides the same functionality as REX.W. VEX.W field can provide completely different functionality for other instructions."

The function AddRexWPrefix(instruction ins, code_t code) https://github.com/dotnet/coreclr/blob/master/src/jit/emitxarch.cpp#L350, takes care of both VEX.W bit and REX.W bit. From this aspect, it's OK to let TakesRexWPrefix return true for vpermq and use AddRexWPrefix to set VEX.W bit.

We do have encodings that use the VEX.W prefix, and it is set based on the opcode size.

For non-integer register, VEX.W bit is just a general opcode extension bit. So I think we need to set the VEX.W bit explicitly for vpermq because vpermq uses ymm or xmm.

I think we have two options here:
(1) reuse TakesRexWPrefix and AddRexWPrefix by letting TakesRexWPrefix return true for vpermq.
(2) define two new functions (e.g. TakesVexWPrefix and AddVexWPrefix) to handle VEX.W bit explicitly. Meanwhile, clean the code in AddRexWPrefix so that AddRexWPrefix only handles REX.W bit.

Any suggestions?

BruceForstall · 2017-04-04T21:23:55Z

src/jit/emitxarch.cpp

@@ -3786,7 +3790,7 @@ void emitter::emitIns_R_R_I(instruction ins, emitAttr attr, regNumber reg1, regN
    instrDesc* id = emitNewInstrSC(attr, ival);

    // REX prefix
-    if (IsExtendedReg(reg1, attr) || IsExtendedReg(reg2, attr))
+    if (IsExtendedReg(reg1, attr) || IsExtendedReg(reg2, attr) || TakesRexWPrefix(ins, attr))


|| TakesRexWPrefix(ins, attr) [](start = 63, length = 29)

Did you add this because of vpermq? Why is that necessary? It wouldn't work on x86.

Yes, it's because vpermq. Will find a better way to handle vpermq.

BruceForstall · 2017-04-04T21:24:57Z

src/jit/emitxarch.cpp

+* Arguments:
+*    ins - the instruction to add
+*    targetReg - the target (destination) register
+*    reg1      - the first source register


Please include all arguments, including attr

BruceForstall · 2017-04-04T21:51:47Z

src/jit/emitxarch.cpp

+            {
+                code = AddRexWPrefix(ins, code);
+            }
+
            if (TakesVexPrefix(ins))


Why was this change necessary? It doesn't look like you added support for any instructions that take a REX prefix.

This is also because of vpermq. Will address vpermq.

@BruceForstall - for AVX instructions that encode REX-equivalent bits (e.g. the W bit), we handle them as for the REX encoding. In fact, for instructions that have both AVX and SSE encodings, we simply generate the AVX encoding from the same encoding bits when AVX encodings are enabled. So, this is the universal way to set 'W' bits, whether they wind up in an AVX or VEX prefix.
I find this approach preferable to adding a separate method. It is what I did for the vgather variants when I was experimenting with that, which also require both the W and L bits to be set appropriately.

@helloguo - thanks for making this change; I think it is a better approach.

BruceForstall · 2017-04-04T22:15:06Z

src/jit/simd.cpp

+        case SIMDIntrinsicConvertToUInt32:
+        case SIMDIntrinsicConvertToInt64:
+        case SIMDIntrinsicConvertToUInt64:
+        {


These look the same as SIMDIntrinsicCast. Why not share that code, above?

BruceForstall · 2017-04-04T22:19:37Z

src/jit/simd.cpp

+        case SIMDIntrinsicNarrow:
+        {
+#ifdef _TARGET_AMD64_
+            assert(!instMethod);


Why is this (and the other ones you've added) under _TARGET_AMD64_? I would remove the #ifdef and make it all platform (namely, x64 and x86, which are the only ones defining FEATURE_SIMD today).

This PR does x64 first. It's a good point to make it all platform. We may create an issue to follow it up.

I think you need to make it all platform before this change is merged. We don't want to advance the two platforms independently; we want them to support the same functionality. Plus, you need to change the VectorConvert tests to check for actually generating accelerated code, and we don't want per-architecture tests.

BruceForstall · 2017-04-04T22:26:19Z

src/jit/simd.cpp

+            GenTree* dupOp1    = fgInsertCommaFormTemp(&op1, gtGetStructHandleForSIMD(simdType, baseType));
+
+            // Widen the lower half and assign it to dstAddrLo.
+            simdTree = gtNewSIMDNode(simdType, op1, nullptr, SIMDIntrinsicWidenLo, baseType, size);


op1 [](start = 47, length = 3)

Should this refer to dupOp1 instead of op1?

@Brucefo - I don't think so; I believe that fgInsertCommaFormTemp will update op1 to point to the comma expression, and return a new node to reference the temp.

In reply to: 109793048 [](ancestors = 109793048)

BruceForstall · 2017-04-04T22:36:57Z

src/jit/simd.cpp

+            hiAsg->gtFlags |= ((simdTree->gtFlags | dstAddrHi->gtFlags) & GTF_ALL_EFFECT);
+
+            retVal = gtNewOperNode(GT_COMMA, simdType, loAsg, hiAsg);
+#else


So it looks like we never create a SIMDIntrinsicWiden tree node, but instead "decompose" it into a high and low part that we put into a GT_COMMA like:

GT_COMMA( *dstHi = SIMDIntrinsicWidenHi(op1) , *dstLo = SIMDIntrinsicWidenLow(op1) )

This seems a bit weird to me, as we normally use commas to create temps in the tree, e.g.:

GT_COMMA( tmp = ... , tmp )

where the "result" of the comma is the tmp on the right-hand-side (second operand) of the GT_COMMA. That would imply the "value" represented by your tree is the *dstLo assignment, and the left side is "just" side-effect.

I wonder if you should instead create some kind of "merge" node instead, e.g.:

SIMDIntrinsicWiden( *dstHi = SIMDIntrinsicWidenHi(op1) , *dstLo = SIMDIntrinsicWidenLow(op1) )

where the SIMDIntrinsicWiden codegen would be a nop, since the operands would do the work for it.

Comments?

Sounds good. Will make the change.

Tried to do something like:

SIMDIntrinsicWiden( *dstHi = SIMDIntrinsicWidenHi(op1) , *dstLo = SIMDIntrinsicWidenLow(op1) )

But it's not that straightforward. I define a new node named SIMDIntrinsicWidenRet, which takes loAsg and hiAsg as inputs. However, before it gets lowered and emitted, assertion happens at lir.cpp line 1503 https://github.com/dotnet/coreclr/blob/master/src/jit/lir.cpp#L1503. I guess because the root node is SIMDIntrinsicWidenRet, it checks the IR from root to leaf. Can you give me any suggestions?

Since both loAsg and hiAsg are assignment already, it may be reasonable to put comma there?

ok, I withdraw my suggestion. Go ahead and stick with your original GT_COMMA implementation.

BruceForstall · 2017-04-04T22:38:28Z

src/jit/lsraxarch.cpp

+            info->srcCount               = 1;
+            info->internalIntCount       = 1;
+            if (comp->getSIMDInstructionSet() == InstructionSet_AVX || simdTree->gtSIMDBaseType == TYP_ULONG)
+            {


coding style: please parenthesize sub-expressions (here and below)

BruceForstall · 2017-04-04T22:41:50Z

src/jit/simdcodegenxarch.cpp

+//----------------------------------------------------------------------------------
+// genSIMDLo64BitConvert: "Generate code to convert lower-most 64-bit item (long <--> double)
+//
+// Arguments:


Please fill this in.

nit: extra double quote in title line

BruceForstall · 2017-04-04T22:43:19Z

Why are these not enabled for x86?

BruceForstall · 2017-04-04T22:51:48Z

src/jit/simdcodegenxarch.cpp

+        regNumber tmpReg2 = genRegNumFromMask(tmpRegsMask);
+        assert(tmpReg != op1Reg && tmpReg2 != op1Reg);
+
+        // tmpReg:      mask, which contains multiple 0x53000000


which contains multiple 0x53000000 [](start = 30, length = 34)

What does "which contains multiple 0x53000000" mean?

Maybe it would be helpful to have a comment showing a sample code sequence that you intend to generate.

In reply to: 109797093 [](ancestors = 109797093)

Sure. Will add comment.

BruceForstall · 2017-04-04T22:55:00Z

src/jit/simdcodegenxarch.cpp

+//
+// Notes:
+//    There are no instructions for converting to/from 64-bit integers, so for these we
+//    do the convertion an element at a time.


convertion [](start = 13, length = 10)

typo: convertion

BruceForstall

Can you address my questions?

helloguo · 2017-04-05T16:03:08Z

@BruceForstall Thanks for your feedback. Will make the change.

BruceForstall · 2017-04-05T17:02:56Z

@helloguo You should rewrite the VectorConvert.cs tests to use the JitLog test infrastructure that ensures that the given functions are accelerated, and make sure every intrinsic you've created is accelerated. Check VectorAdd.cs for example. You might also be able to use CheckValue<> instead of your own value comparisons.

russellhadley · 2017-04-10T16:44:26Z

@BruceForstall, @helloguo Just looping back to this. Are we in shape to get the AMD64 changes merged? Has the follow up item for the X86 work been opened?

BruceForstall · 2017-04-10T16:50:24Z

@russellhadley I haven't re-reviewed. I don't want it merged until x86 is fully supported equal to amd64.

russellhadley · 2017-04-10T16:56:07Z

Why do we need to wait? Doing it in two steps doesn't seem problematic to me.

BruceForstall · 2017-04-10T17:01:52Z

@russellhadley Why do we need to rush it and not do it right at the first merge?

russellhadley · 2017-04-10T17:04:02Z

@BruceForstall I don't see it as a rush, just more bake time and checking in early and often. I'm not getting how the big bang is more "right" than in stages.

BruceForstall · 2017-04-10T18:18:34Z

@russellhadley As a matter of principle, x86 and x64 have feature parity, including w.r.t. SIMD intrinsics. We don't want to regress that, even temporarily. (I can't be assured that any resources devoted to adding intrinsics don't disappear after merging 1/2 of the work.) As a practical matter, the VectorConvert test here needs to be rewritten to use JitLog to verify that intrinsics were used. We don't want to disable this test for x86 -- we want all tests running on all architectures.

russellhadley · 2017-04-10T18:29:04Z

@BruceForstall There will be feature parity, just one will be slower than the other (i.e. X86 not use the intrinsics initially). In terms of resource allocation I don't think PRs are the appropriate place for that discussion. From my side I'm willing to approve the merge if the amd64 changes if they are correct and meet the design direction.

CarolEidt · 2017-04-17T16:54:15Z

src/jit/simdcodegenxarch.cpp

+        // tmpReg = 0
+        // tmpReg = (0 > targetReg)           // (If signed) Get the sign bits
+        // punpck[l|h]dq  targetReg, tmpReg   // Interleave the sign bits
+        regNumber tmpReg = genRegNumFromMask(simdNode->gtRsvdRegs & RBM_ALLFLOAT);


This comment is not correct - it needs to reflect the code being generated below.

BruceForstall · 2017-05-01T21:52:36Z

src/jit/lsraxarch.cpp

+            }
+            else if ((comp->getSIMDInstructionSet() == InstructionSet_AVX) || (simdTree->gtSIMDBaseType == TYP_ULONG))
+#endif
+            {


Couldn't this be:

#ifdef _TARGET_X86_ if (simdTree->gtSIMDBaseType == TYP_LONG) { info->internalFloatCount = 3; } else #endif if ((comp->getSIMDInstructionSet() == InstructionSet_AVX) || (simdTree->gtSIMDBaseType == TYP_ULONG))

?

BruceForstall · 2017-05-01T21:54:32Z

src/jit/simd.cpp

+            retVal   = simdTree;
+#else
+            JITDUMP("SIMD Conversion is not supported on this platform\n");
+            return nullptr;


Can you be more specific here, since ConvertToSingle/Double/Int32/UInt32 is supported. E.g.,

JITDUMP("SIMD Conversion to Int64/UInt64 is not supported on this platform\n");

BruceForstall · 2017-05-01T21:58:56Z

src/jit/simdcodegenxarch.cpp

+    if (intrinsicID == SIMDIntrinsicConvertToSingle && baseType == TYP_UINT)
+    {
+        regNumber tmpIntReg   = genRegNumFromMask(simdNode->gtRsvdRegs & RBM_ALLINT);
+        regMaskTP tmpRegsMask = (simdNode->gtRsvdRegs & RBM_ALLFLOAT);


Please change all your code that finds/extracts registers from the gtRsvdRegs mask to instead call the ExtractTempReg() or GetSingleTempReg() APIs. You can use AvailableTempRegCount() for asserts, if desired, although generally just using the new APIs will give you the asserts you need.

Thank you for your suggestion. Because these APIs (ExtractTempReg() or GetSingleTempReg()) were not there when this PR was first made, it's not very convenient to fix it in this PR. I will submit a fix once this PR is merged.

Couldn't you just rebase to upstream/master, make the change, then push your branch again?

Sure. Done.

Looks great. Thanks.

BruceForstall · 2017-05-01T22:00:54Z

src/jit/simdcodegenxarch.cpp

+    }
+    else if (iset == InstructionSet_AVX || (baseType == TYP_ULONG))
+#endif
+    {


This could be:

#ifdef _TARGET_X86_ if (baseType == TYP_LONG) { ... } #else else if (iset == InstructionSet_AVX || (baseType == TYP_ULONG))

CarolEidt · 2017-05-01T22:22:35Z

Because TakesRexWPrefix returns false when attris not EA_8BYTE (https://github.com/dotnet/coreclr/blob/master/src/jit/emitxarch.cpp#L222), we need to set attr to EA_8BYTEfor vpermq in order to set VEX.W bit.

Yes, I believe that's what I suggested. Note that vpermq is specifically for quadwords, which in xarch-speak is 8 bytes. So it makes sense to provide EA_8BYTE as the emitSize on the call to emitIns_R_R_I. (See my comment in simdcodegenxarch.cpp. I don't think we need (or want) a special case in TakesRexWPrefix().

helloguo · 2017-05-02T17:30:38Z

Yes, I believe that's what I suggested. Note that vpermq is specifically for quadwords, which in xarch-speak is 8 bytes. So it makes sense to provide EA_8BYTE as the emitSize on the call to emitIns_R_R_I. (See my comment in simdcodegenxarch.cpp. I don't think we need (or want) a special case in TakesRexWPrefix().

@CarolEidt if we just set attr to EA_8BYTE, the L bit will not be set to 1 (https://github.com/dotnet/coreclr/blob/master/src/jit/emitxarch.cpp#L197-L200), which means we need a special case for vpermq in function AddVexPrefix.

CarolEidt · 2017-05-03T16:18:13Z

@helloguo - Sorry for the delay. I spent some time looking into this, and you are right; it doesn't seem that there is a great way to cleanly support both register size specification and operand size specification in the current instruction formats and descriptors. I looked at what I did for the vgather variants (experimental work for https://github.com/dotnet/corefx/issues/1608), which have a similar encoding issue - the 'W' bit encodes single vs. double, while the 'L' bit (which in this case may take on both 0/1) encodes 128 vs. 256. Since the opcode implicitly encodes single vs. double, I had taken an approach similar to what you suggest above in TakesRexWPrefix().

I suggest adding a very explicit comment along these lines:

// Because the current implementation of AVX does not have a way to distinguish between the register
// size specification (128 vs. 256 bits) and the operand size specification (32 vs. 64 bits), where both are
// required, the instruction must be created with the register size attribute (EA_16BYTE or EA_32BYTE),
// and here we must special case these by the opcode.

Does that seem reasonable?

russellhadley · 2017-05-03T16:45:14Z

@pgavlin Can you add a note on the complexity of keeping the register and operand size specification separate?

BruceForstall · 2017-05-03T23:46:49Z

The test changes look good to me.

CarolEidt

Looks good to me.

CarolEidt · 2017-05-04T00:32:55Z

src/jit/emitxarch.cpp

+            {
+                code = AddRexWPrefix(ins, code);
+            }
+
            if (TakesVexPrefix(ins))


@BruceForstall - for AVX instructions that encode REX-equivalent bits (e.g. the W bit), we handle them as for the REX encoding. In fact, for instructions that have both AVX and SSE encodings, we simply generate the AVX encoding from the same encoding bits when AVX encodings are enabled. So, this is the universal way to set 'W' bits, whether they wind up in an AVX or VEX prefix.
I find this approach preferable to adding a separate method. It is what I did for the vgather variants when I was experimenting with that, which also require both the W and L bits to be set appropriately.

CarolEidt · 2017-05-04T00:33:52Z

src/jit/emitxarch.cpp

+            {
+                code = AddRexWPrefix(ins, code);
+            }
+
            if (TakesVexPrefix(ins))


@helloguo - thanks for making this change; I think it is a better approach.

helloguo · 2017-05-04T04:15:39Z

@dotnet-bot test Windows_NT x64 Debug Build and Test

russellhadley · 2017-05-04T21:38:08Z

Given the dates for 2.0 (we're going to fork imminently), let's hold off on this until master opens for 2.1.

BruceForstall · 2017-05-10T22:10:37Z

@helloguo I expect we will be able to merge this soon. In the meantime, can you update the change so it doesn't have any merge conflicts?

BruceForstall · 2017-05-10T22:53:10Z

@helloguo Looks like GitHub still thinks there is a merge conflict in codegenlinear.h? Maybe you need to rebase to upstream/master?

… and x86, except double->long/ulong conversion on x86

helloguo · 2017-05-10T23:26:19Z

@dotnet-bot test Ubuntu arm Cross Release Build

helloguo · 2017-05-11T00:42:44Z

@BruceForstall Done.

BruceForstall · 2017-05-19T00:26:43Z

@dotnet-bot test this please

helloguo · 2017-05-19T05:40:06Z

@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test

BruceForstall · 2017-05-19T17:25:08Z

@helloguo Thanks for all the work!

dnfclas added the cla-already-signed label Apr 3, 2017

helloguo mentioned this pull request Apr 3, 2017

Vector conversion support #9920

Closed

BruceForstall self-requested a review April 3, 2017 18:40

helloguo force-pushed the VectorConversion branch from bb76ccf to ab20f5f Compare April 3, 2017 19:23

BruceForstall reviewed Apr 4, 2017

View reviewed changes

BruceForstall suggested changes Apr 4, 2017

View reviewed changes

CarolEidt reviewed Apr 17, 2017

View reviewed changes

BruceForstall reviewed May 1, 2017

View reviewed changes

CarolEidt approved these changes May 4, 2017

View reviewed changes

helloguo force-pushed the VectorConversion branch from 054440d to 8d9b587 Compare May 4, 2017 18:51

helloguo changed the title ~~Add JIT intrinsics support for vector conversion on AMD64~~ Add JIT intrinsics support for vector conversion on AMD64 and x86 May 4, 2017

BruceForstall approved these changes May 4, 2017

View reviewed changes

russellhadley added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label May 4, 2017

helloguo force-pushed the VectorConversion branch from 24148af to 14a2812 Compare May 10, 2017 22:38

add jit intrinsic support for vector conversion/narrow/widen on AMD64…

965d5ee

… and x86, except double->long/ulong conversion on x86

helloguo force-pushed the VectorConversion branch from 14a2812 to 965d5ee Compare May 10, 2017 22:57

BruceForstall removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label May 19, 2017

BruceForstall merged commit 7a75598 into dotnet:master May 19, 2017

karelz modified the milestone: 2.0.0 Aug 28, 2017

Add JIT intrinsics support for vector conversion on AMD64 and x86 #10662

Add JIT intrinsics support for vector conversion on AMD64 and x86 #10662

Conversation

helloguo commented Apr 3, 2017

helloguo commented Apr 3, 2017

helloguo commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helloguo Apr 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall left a comment

Choose a reason for hiding this comment

helloguo commented Apr 5, 2017

BruceForstall commented Apr 5, 2017

russellhadley commented Apr 10, 2017

BruceForstall commented Apr 10, 2017

russellhadley commented Apr 10, 2017

BruceForstall commented Apr 10, 2017

russellhadley commented Apr 10, 2017

BruceForstall commented Apr 10, 2017

russellhadley commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarolEidt commented May 1, 2017

helloguo commented May 2, 2017

CarolEidt commented May 3, 2017

russellhadley commented May 3, 2017

BruceForstall commented May 3, 2017

CarolEidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helloguo commented May 4, 2017

russellhadley commented May 4, 2017

BruceForstall commented May 10, 2017

BruceForstall commented May 10, 2017

helloguo commented May 10, 2017

helloguo commented May 11, 2017

BruceForstall commented May 19, 2017

helloguo commented May 19, 2017

BruceForstall commented May 19, 2017

helloguo Apr 18, 2017 •

edited