Updating the HWIntrinsic codegen to support marking LoadVector128 and LoadAlignedVector128 as contained. #16095

tannergooding · 2018-01-30T05:25:23Z

tannergooding · 2018-01-30T05:26:21Z

This is a basic example of marking a HWIntrinsic node as contained.

I don't think it should be merged until after #15771, so that we can readily get more test coverage on this (15771 adds some explicit containment tests, and will make it simpler to add others).

tannergooding · 2018-01-30T05:27:35Z

src/jit/hwintrinsiccodegenxarch.cpp

@@ -230,6 +230,11 @@ void CodeGen::genHWIntrinsic_R_R_RM(GenTreeHWIntrinsic* node, instruction ins)

            compiler->tmpRlsTemp(tmpDsc);
        }
+        else if (op2->OperIsHWIntrinsic())


Similar code is needed in genHWIntrinsic_R_R_RM_I.

tannergooding · 2018-01-30T05:42:41Z

Base:

C4E1791038           vmovupd  xmm7, xmmword ptr [rax]
C4614858C7           vaddps   xmm8, xmm6, xmm7

Diff:

C4E1485838           vaddps   xmm7, xmm6, xmmword ptr [rax]

fiigii · 2018-01-30T05:52:58Z

src/jit/lowerxarch.cpp

+//
+bool Lowering::IsContainableHWIntrinsicOp(GenTree* node)
+{
+    if (!node->OperIsHWIntrinsic())


I think this earlier return can be an assertion.

I'm not positive. Currently this is called for any op2 which is not a containable memory op. I don't know if that will always be a HWIntrinsic node.

I think, for example, a user defined function which returns a Vector128<T>, would fail such an assertion.

I see. Thanks.

I've not tested that, however, and @CarolEidt or @mikedn might know for certain

Seems fine to me as it is. That's how isContainableMemoryOp does it as well.

tannergooding · 2018-01-30T06:12:24Z

src/jit/lowerxarch.cpp

+    switch (intrinsicID)
+    {
+        case NI_SSE_LoadAlignedVector128:
+        case NI_SSE_LoadVector128:


LoadScalarVector128 should also be handled here, but I need to make sure it is emitted properly (since it is a m32 instead of a m128)

but I need to make sure it is emitted properly (since it is a m32 instead of a m128)

Do you mean only folding AddScalar(v1, LoadScalarVector128(address)) but not folding AddScalar(v1, LoadVector128(address))?

Well specifically for Add(v1, LoadScalar(address)) , which would cause a read of too many bits, if folded.

I thin the other (AddScalar(v1, LoadVector128(address)) ) is safe to fold (we should still determine if we want to do such an optimization) since the read isn't stored and the upper bits don't impact the final result

We may not want to make the second optimization since the full read has potentially observable side effects (it could cause an AV exception, if you read past the end of an array for example or if it was an aligned read of an unlined address, etc).

tannergooding · 2018-01-30T15:44:34Z

@CarolEidt, @fiigii.

Folding LoadAligned, on newer hardware, might need a slightly more in-depth discussion and definitely needs an explicit design decision. Folding a 128-bit load into a scalar instruction needs a similar discussion/decision.

Folding LoadAligned

LoadAligned has an observable side-effect in that it will cause an AccessViolation if you pass it an unaligned address.

When using the non-VEX encoding (generally legacy hardware), INS xmm, [mem], the memory location must be aligned or the instruction will also raise an AccessViolation exception. So it is fine to fold LoadAligned here (and it is the only thing we can fold).

However, when using the VEX-encoding (newer hardware that supports AVX or AVX2), INS xmm, xmm, [mem], the memory location can be aligned or unaligned and no AccessViolation exception is raised. As such, we can fold regular Loads or LoadAligneds, however folding the latter can cause an observable change in side-effects.

For example, on the below, if you are on newer hardware, and the second load is not marked as contained, you will get an AV. However, if it is contained, the code will "just work".

var value = Sse.LoadAlignedVector128(address);
var result = Sse.Add(value, Sse.LoadAlignedVector128(address + 2));

Folding larger reads

For the scalar instructions, the address never needs to be aligned (on newer or older hardware) and is only a partial read (m8, m16, m32 or m64) rather than a full read (m128).

If a user were to use a full read, and pass it directly into an operation, there would be a chance to fold it.

For example, on the below, the second load will normally read the full 128-bits. However, the value is never stored and the upper bits are not used in the operation. As such, it is "safe" to fold this operation. However, this has potentially observable side-effects with things like caching, page loads, AV (if the full read would extend past the end of an array or page boundary, etc).

var value = Sse.LoadScalarVector128(address);
var result = Sse.AddScalar(value, Sse.LoadVector128(address + 1));

mikedn · 2018-01-30T16:10:22Z

For example, on the below, if you are on newer hardware, and the second load is not marked as contained, you will get an AV. However, if it is contained, the code will "just work".

I doubt that the fact that the code will just work is an issue. That is, it's unlikely that people will use LoadAligned to check if the data is aligned.

For the scalar instructions, the address never needs to be aligned (on newer or older hardware) and is only a partial read (m8, m16, m32 or m64) rather than a full read (m128).

I'm surprised that this is even up for discussion. Reading more memory than requested it's a very dubious thing to do and should only be done in very specific circumstances.

tannergooding · 2018-01-30T16:18:13Z

I doubt that the fact that the code will just work is an issue. That is, it's unlikely that people will use LoadAligned to check if the data is aligned.

Yes, but it is an observable side-effect, which is something we normally don't allow you to hide (as has been stated in at least a couple other issues where I, or others, have asked for better codegen/inlining 😄).

I'm surprised that this is even up for discussion. Reading more memory than requested it's a very dubious thing to do and should only be done in very specific circumstances.

I imagine we should just not fold these, but I want an explicit decision so that I can document it in IsContainableHWIntrinsicOp.

mikedn · 2018-01-30T16:25:20Z

Yes, but it is an observable side-effect, which is something we normally don't allow you to hide (as has been stated in at least a couple other issues where I, or others, have asked for better codegen/inlining 😄).

That's not really a side effect in the common sense. It's not as if you're transforming a null pointer dereference in a 0.

I imagine we should just not fold these, but I want an explicit decision so that I can document it in IsContainableHWIntrinsicOp.

It's the other way around. You would need an explicit decision to generate wider loads.

tannergooding · 2018-01-30T16:33:13Z

That's not really a side effect in the common sense. It's not as if you're transforming a null pointer dereference in a 0.

I think any generated exception is an observable side effect according to the spec. In either way, getting an official ruling would be good.

NOTE: I am in favor of folding these, but possibly only in release/optimized code (and not in debug/min-opts code).

It's the other way around. You would need an explicit decision to generate wider loads.

Why? The user coded AddScalar(value, LoadVector128(address)), this means the user stated: Do a wide load. It should be an explicit decision to allow shrinking such a load (since the folded operation normally only reads a m32, but the unfolded operation would read m128).

For the other end, Add(value, LoadScalarVector128(address)). I don't think we can safely fold (since the folded operation reads a m128, but the unfolded operation only reads m32), as it could cause reading across a page/cache-line boundary, cause an AV exception, read past the end of an array, etc.

CarolEidt · 2018-01-30T16:44:16Z

Obviously I agree that we can't fold something like Add(value, LoadScalarVector128(address)) as it could cause an exception that would not have otherwise occurred (not to mention that I believe it would get a different answer)

However, I am also not in favor of any folding that would suppress an exception that would otherwise have happened. Getting optimal code may require the developer to check for (and use) higher-level ISA instructions to get the folding, but I think that's a reasonable expectation given that we already expect the developer to have a detailed knowledge of the hardware - this is, after all, effectively inline assembly.

mmitche · 2018-01-30T16:57:37Z

@dotnet-bot test this please

tannergooding · 2018-01-30T17:07:42Z

However, I am also not in favor of any folding that would suppress an exception that would otherwise have happened. Getting optimal code may require the developer to check for (and use) higher-level ISA instructions to get the folding, but I think that's a reasonable expectation given that we already expect the developer to have a detailed knowledge of the hardware - this is, after all, effectively inline assembly.

My concern on this is that for addps xmm, xmm, we only expose Sse.Add and we don't redeclare Add(Vector128<float>, Vector128<float>) for AVX. Instead Sse.Add emits the VEX encoding on a hardware that supports it and the non-VEX encoding on older hardware (or certain newer hardware that doesn't support AVX). For Avx, we only expose the Add(Vector256<float>, Vector256<float>) (addps ymm, ymm) which, of course, is a larger read/write and would require a different algorithm.

If a user has already validated that the loads will be aligned, they shouldn't need to write two versions of the algorithm (one that uses LoadAligned on older hardware and one that uses Load on newer hardware) so that folding happens in both places.

That being said, this is a perfect use-case for Assume.Alignment() (see https://github.com/dotnet/corefx/issues/26188), which would allow the JIT to fold if optimizations are enabled.

tannergooding · 2018-01-30T17:15:12Z

src/jit/lowerxarch.cpp

+        // Only fold a scalar load into a SIMD scalar intrinsic to ensure the number of bits
+        // read remains the same. Likewise, we can't fold a larger load into a SIMD scalar
+        // intrinsic as that would read fewer bits that requested.
+        case NI_SSE_LoadScalar:


Probably want to assert here that the baseType of the containing node is the same as the baseType of the load.

It should already line up due to the method signatures, but we don't want to end up in a scenario where the LoadScalar is for m32, but the containing node would do a m64.

mikedn · 2018-01-30T17:45:12Z

Why? The user coded AddScalar(value, LoadVector128(address))

That one is fine, there's nothing interesting about containment in this case. I was talking about the case where a wider load would be generated. Narrowing loads can also be problematic if the data is not aligned, you could end up hiding an invalid memory access.

mikedn · 2018-01-30T17:52:02Z

However, I am also not in favor of any folding that would suppress an exception that would otherwise have happened.

AFAIR native compilers have no problem doing this. It's not clear why it would be problem in .NET. Maybe because the intrinsic names are somewhat different in regards to alignment, in C/C++ you basically have Load and LoadUnaligned, not Load and LoadAligned.

mikedn · 2018-01-30T17:57:12Z

If a user has already validated that the loads will be aligned, they shouldn't need to write two versions of the algorithm (one that uses LoadAligned on older hardware and one that uses Load on newer hardware) so that folding happens in both places.

Indeed, they shouldn't need to write 2 versions. Because the Load version would be good enough for current hardware.

tannergooding · 2018-01-30T17:58:01Z

It's not clear why it would be problem in .NET.

Because LoadAligned is one of the few intrinsic instructions that has an exception behavior (this is excluding the floating-point exceptions we mask by default, as per the spec).

It's implementation is effectively:

if ((address % 16) != 0)
{
    throw new AccessViolationException();
}

return LoadUnaligned(address);

This is in the same vein as why we can't transform:

int Select(bool condition, int trueValue, int falseValue)
{
    return condition ? trueValue : falseValue;
}

int MyMethod(bool condition, int[] a, int[] b)
{
    return Select(condition, a[5], b[6]);
}

into:

int MyMethod(bool condition, int[] a, int[] b)
{
    return condition ? a[5] : b[6];
}

even though the native compiler can. Since it would mask a NRE if a was null, but condition was false

tannergooding · 2018-01-30T18:00:39Z

Indeed, they shouldn't need to write 2 versions. Because the Load version would be good enough for current hardware.

There are modern processors (such as the Kaby Lake Celeron/Pentiums -- https://ark.intel.com/products/97460/Intel-Pentium-Processor-G4620-3M-Cache-3_70-GHz), which don't support VEX encoding.

mikedn · 2018-01-30T18:12:20Z

This is in the same vein as why we can't transform:

Are you really going to use that as an argument? Really really?

There are modern processors (such as the Kaby Lake Celeron/Pentiums -- ) which don't support VEX encoding

And let me guess - people who use such processors expect to get best performance from a gimped
processor and are the primary targets for .NET Core apps. Or .NET apps in general. Not to mention that containment in itself won't necessarily make a significant difference. Contained or not, a memory load is a memory load. This is mainly a code size optimization and it may very well not make any difference in many cases.

tannergooding · 2018-01-30T18:24:11Z

Are you really going to use that as an argument? Really really?

Yes :)

It was made very clear in my proposal to allow the folding of such things why we can't. Folding LoadAligned is the exact same scenario, it would mask an exception that would otherwise be thrown.

The solution, I believe, is to move forward with a proposal such https://github.com/dotnet/corefx/issues/26188, which did garner support, but which required a reasonable use case (such as this) and a prototype to back it up.

And let me guess - people who use such processors expect to get best performance from a gimped
processor and are the primary targets for .NET Core apps. Or .NET apps in general.

Regardless of whether or not you view the processor as "gimped", some people will end up getting processors like that or won't upgrade every new processor generation and some of those people will run .NET.

That alone isn't enough of a case to argue for allowing it, without some more concrete numbers, but it is a start.

Not to mention that containment in itself won't necessarily make a significant difference. Contained or not, a memory load is a memory load. This is mainly a code size optimization and it may very well not make any difference in many cases.

Code size is also important for throughput in performance oriented code (such as the general use-case for HWIntrinsics). If you end up doubling the code size, because Loads aren't folded, it can make a measurable difference in the total execution time, especially if that code is on a hot-path.

CarolEidt

LGTM with one optional suggestion

CarolEidt · 2018-01-31T17:27:22Z

src/jit/codegenlinear.cpp

@@ -1297,6 +1297,10 @@ void CodeGen::genConsumeRegs(GenTree* tree)
        {
            genConsumeReg(tree->gtGetOp1());
        }
+        else if (tree->OperIsHWIntrinsic())


It seems like this would be a good place to assert that HW_Flag_NoContainment is not set, or that IsContainableHWIntrinsicOp(tree, tree->gtGetOp1()) but perhaps that's overkill.

I think asserting is a good idea. I'll update.

This is also pending #16114 and #16115. The former to fixup the various flags and the latter to make it easier to add some tests with this change (we currently don't have any tests that use Load, LoadScalar, or LoadAligned in a way that they could be contained).

Hmm, actually, I can't easily check IsContainableHWIntrinsicOp.

It is a member of Lowering and depends on IsContainableMemoryOp, which itself depends on m_lsra.

I would need to make m_pLowering visible from compiler somewhere to use that check.

The other check, flagsOfHWIntrinsic, currently requires friend access to the compiler, but I don't see any reason why those can't be public.

@CarolEidt, what would you recommend here?

I can't recall why IsContainableMemoryOp has its actual implementation on LinearScan when all the containment analysis is done in Lowering (even before we moved the TreeNodeInfoInit to LSRA). In any case, it seems like something we might want to check during code generation, so we may want to expose it in some way. That said, I don't feel that it's critical.

On the other hand, the flagsOfHwIntrinsic I think should definitely be public so that they are accessible from codegen.

I can't recall why IsContainableMemoryOp has its actual implementation on LinearScan when all the containment analysis is done in Lowering (even before we moved the TreeNodeInfoInit to LSRA).

Looks to be one use in LSRA, for SIMDIntrinsicGetItem: https://github.com/dotnet/coreclr/blob/master/src/jit/lsraxarch.cpp#L3026

In any case, it seems like something we might want to check during code generation, so we may want to expose it in some way. That said, I don't feel that it's critical.

Exposing a getLowering() method in the compiler is easy enough, but it also requires us to #include "lower.h" somewhere where codegen can pick it up (probably just in codegen.h). I've commented out this particular assert with a TODO and will finish looking tomorrow if I get the chance.

On the other hand, the flagsOfHwIntrinsic I think should definitely be public so that they are accessible from codegen.

It is already available to codgen, I just forgot to put the assert in an #ifdef _TARGET_XARCH_ since its in codegenlinear. 😄

Both asserts I tried to add here would have been invalid. tree is the node which was contained, we would want to be doing the assert on the node which contains tree.

Moved the asserts to hwintrinsiccodegenxarch.cpp.

tannergooding · 2018-02-01T02:34:09Z

The product code here should be "complete". I'd still like to wait on #16115 and add some explicit tests before merging, however.

tannergooding · 2018-02-01T23:38:47Z

Updated the templated tests to also validate using LoadVector and LoadAlignedVector works. We should eventually add tests that use LoadScalarVector as well, but that will require slightly more complex changes to the template.

tannergooding · 2018-02-02T00:30:35Z

src/jit/hwintrinsicxarch.cpp

+
+    // Set `compFloatingPointUsed` to cover the scenario where an intrinsic is being on SIMD fields, but
+    // where no SIMD local vars are in use. This is the same logic as is used for FEATURE_SIMD.
+    compFloatingPointUsed = true;


FYI. @CarolEidt, @fiigii

This is the same logic as https://github.com/dotnet/coreclr/blob/master/src/jit/simd.cpp#L3094. An assert was being hit in LSRA for reflection calls which used LoadVector128.

This needs to be cleaned up to use a flag so we aren't setting it on intrinsics which don't actually use any floating-point nodes/registers (such as Crc32).

Also FYI. @sdmaclea. This appears to impact ARM64 as well (based on the FEATURE_SIMD code), and you may need something similar.

I also imagine various code could be updated elsewhere in the JIT to properly support TYP_SIMD as floating-point registers, rather than requiring this special handling here.

tannergooding · 2018-02-02T04:29:37Z

src/jit/hwintrinsiccodegenxarch.cpp

@@ -242,7 +250,6 @@ void CodeGen::genHWIntrinsic_R_R_RM(GenTreeHWIntrinsic* node, instruction ins)
                    offset = 0;

                    // Ensure that all the GenTreeIndir values are set to their defaults.
-                    assert(memBase->gtRegNum == REG_NA);


This was found to be incorrect in the larger emitInsBinary refactoring, but wasn't also removed from here.

tannergooding · 2018-02-02T04:44:11Z

@CarolEidt, @fiigii. Could you give another review pass when you get the opportunity.

I think I'm satisfied with the changes now.

We should have test coverage over most of the containment scenarios
- Explicit tests that use LoadScalar still need coverage, but require a different template (not applicable to Vector256)
- Explicit tests that cover containment for the commutative case are still needed, but again require a different template
We should have asserts in codegen to validate the containment checks are as expected.

CarolEidt

LGTM - thanks!

CarolEidt · 2018-02-02T17:05:25Z

src/jit/hwintrinsiccodegenxarch.cpp

@@ -215,6 +215,9 @@ void CodeGen::genHWIntrinsic_R_R_RM(GenTreeHWIntrinsic* node, instruction ins)

    if (op2->isContained() || op2->isUsedFromSpillTemp())
    {
+        assert((Compiler::flagsOfHWIntrinsic(node->gtHWIntrinsicId) & HW_Flag_NoContainment) == 0);
+        assert(compiler->m_pLowering->IsContainableHWIntrinsicOp(node, op2) || op2->IsRegOptional());


fiigii

Thank you for the work.

tannergooding · 2018-02-02T18:33:53Z

~~Ubuntu test failures are due to the API name fixes and have already been fixed #16169~~ Edit: One of them is due to a change brought in by this, fixing.

tannergooding · 2018-02-02T23:57:19Z

Failures are from SSE2 and AVX tests attempting to use the not yet implemented LoadVector intrinsics for those ISAs, will fix.

fiigii · 2018-02-03T00:10:07Z

SSE2 and AVX tests attempting to use the not yet implemented LoadVector intrinsics

Are you going to disable this behavior? I can implement these Load* intrinsics in this weekend.

tannergooding · 2018-02-03T05:16:51Z

Are you going to disable this behavior?

Yes (effectively).

I am going to revert the test changes for SSE2/AVX (since those just added the Load/LoadAligned tests) and comment those out from generation for the time being.

tannergooding · 2018-02-03T05:33:40Z

Same as before (just squashed into a product changes and a test changes commit), but with the invalid test changes reverted for SSE2/AVX/AVX2 (since they don't have the required Load/LoadAligned intrinsics implemented yet).

… LoadAlignedVector128 as contained.

…dAligned

tannergooding · 2018-02-03T08:17:35Z

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

tannergooding commented Jan 30, 2018

View reviewed changes

fiigii reviewed Jan 30, 2018

View reviewed changes

tannergooding commented Jan 30, 2018

View reviewed changes

mmitche closed this Jan 30, 2018

mmitche reopened this Jan 30, 2018

tannergooding commented Jan 30, 2018

View reviewed changes

CarolEidt approved these changes Jan 31, 2018

View reviewed changes

tannergooding changed the title ~~[WIP] Updating the HWIntrinsic codegen to support marking LoadVector128 and LoadAlignedVector128 as contained.~~ Updating the HWIntrinsic codegen to support marking LoadVector128 and LoadAlignedVector128 as contained. Feb 1, 2018

tannergooding commented Feb 2, 2018

View reviewed changes

CarolEidt approved these changes Feb 2, 2018

View reviewed changes

tannergooding mentioned this pull request Feb 2, 2018

[Arm64] Implement Simd.Insert #16160

Merged

fiigii approved these changes Feb 2, 2018

View reviewed changes

tannergooding added 2 commits February 2, 2018 22:44

Updating the HWIntrinsic codegen to support marking LoadVector128 and…

d430614

… LoadAlignedVector128 as contained.

Updating the templated x86 hwintrinsic tests to validate Load and Loa…

51bd59d

…dAligned

tannergooding merged commit eb54e48 into dotnet:master Feb 3, 2018

tannergooding deleted the sse-intrinsics branch May 30, 2018 04:17

Updating the HWIntrinsic codegen to support marking LoadVector128 and LoadAlignedVector128 as contained. #16095

Updating the HWIntrinsic codegen to support marking LoadVector128 and LoadAlignedVector128 as contained. #16095

Conversation

tannergooding commented Jan 30, 2018

tannergooding commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented Jan 30, 2018

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

tannergooding commented Jan 30, 2018

Folding LoadAligned

Folding larger reads

mikedn commented Jan 30, 2018

tannergooding commented Jan 30, 2018

mikedn commented Jan 30, 2018

tannergooding commented Jan 30, 2018 • edited Loading

CarolEidt commented Jan 30, 2018

mmitche commented Jan 30, 2018

tannergooding commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedn commented Jan 30, 2018

mikedn commented Jan 30, 2018

mikedn commented Jan 30, 2018

tannergooding commented Jan 30, 2018 • edited Loading

tannergooding commented Jan 30, 2018

mikedn commented Jan 30, 2018

tannergooding commented Jan 30, 2018

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

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

tannergooding commented Feb 1, 2018

tannergooding commented Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented Feb 2, 2018

CarolEidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiigii left a comment

Choose a reason for hiding this comment

tannergooding commented Feb 2, 2018 • edited Loading

tannergooding commented Feb 2, 2018

fiigii commented Feb 3, 2018

tannergooding commented Feb 3, 2018

tannergooding commented Feb 3, 2018

tannergooding commented Feb 3, 2018

tannergooding commented Jan 30, 2018 •

edited

Loading

tannergooding commented Jan 30, 2018 •

edited

Loading

tannergooding commented Feb 1, 2018 •

edited

Loading

tannergooding commented Feb 2, 2018 •

edited

Loading