Update x86 HWIntrinsic Tests #15771

tannergooding · 2018-01-06T13:29:00Z

I've updated the x86 hardware intrinsics to share some base code and to follow a template that ensures we get fairly consistent coverage.

Notable, The TestTable and Program.cs are shared.

All tests will be expected to have the following tests:

Basic - Validates basic functionality works as intended
Reflection - Validates invocation via reflection works as intended
Unsupported - Validates PNSE is thrown when IsSupported == false
Containment - Validates that functionality works as intended with various argument types that should be treated as contained
- ClsVar - Static Variables
- LclVar - Local Variables
- LclFld - Field of a local variable
- Fld - Field of the current instance
Other - Validates other functionality that may be test specific (things like commutative or associativity tests can go here, if we need them)

At the very least, the new pattern is very "templatable" and is easy to copy and then do a search/replace on to get working for new tests

tannergooding · 2018-01-06T13:30:57Z

FYI. @fiigii, @jkotas

I'm not tied to this pattern (and it didn't take very long to update, as it was really just copy/paste and search/replace to get working, so if it is "throwaway" it's no big deal), but I think it might work well enough.

tannergooding · 2018-01-06T13:52:58Z

Seems this found a bug under _ro as well.

For all the tests, the following pattern asserts:

var result = Sse.Add(
    Unsafe.Read<Vector128<float>>(_testTable.inArray1Ptr),
    Unsafe.Read<Vector128<float>>(_testTable.inArray2Ptr)
);

However, the following does not:

var left = Unsafe.Read<Vector128<float>>(_testTable.inArray1Ptr);
var right = Unsafe.Read<Vector128<float>>(_testTable.inArray2Ptr);

var result = Sse.Add(
    left,
    right
);

Assertion is:

Assert failure(PID 9644 [0x000025ac], Thread: 7680 [0x1e00]): Assertion failed 'argCnt == methInfo->args.totalILArgs()' in 'HardwareIntrinsicTest.Test:ValidateBasic():this' (IL size 67)
    File: d:\repos\coreclr\src\jit\importer.cpp Line: 18195

JitDump:
JitDump.txt

tannergooding · 2018-01-06T14:02:25Z

For reference: The above only happens for COMPlus_EnableIncompleteISAClass=0 (so Sse.IsSupported returns false).

However, I have Sse.IsSupported indirected through Test.IsSupported, so the JIT doesn't constant fold the other calls away

tannergooding · 2018-01-06T14:09:18Z

Looks like it might be a combination of inlining along with us returning a gtNewMustThrowException node for IsSupported=false: https://github.com/dotnet/coreclr/blob/master/src/jit/hwintrinsicxarch.cpp#L299.

4creators · 2018-01-06T15:40:40Z

tests/src/JIT/HardwareIntrinsics/X86/Shared/Program.cs

+            {
+                var test = new Test();
+
+                if (test.IsSupported)


Split of tests into different calling patterns is very good, however, it could be more convenient to use Func<T> or similar to pass tests into main. This would allow for greater flexibility, since individual test would not have to implement all functions but only pass those which are relevant for them and could implement even not predefined test functions.

The current functions represent the functionality that all tests should probably implement, in the most generic sense.

They ensure that the tests are functionally correct (for the various modes required) and that any new codegen works as intended.

There may be some tests which differ slightly, which is the purpose of the ValidateOther. For tests that differ more significantly, a more custom Main method is fine and would be expected.

The current functions represent the functionality that all tests should probably implement

Yes it is most probably correct but my intention was to suggest using more flexible pattern in discovering and running tests what would lift limitations on test implementation and decouple Program from Test. For instance every test method should implement Func<bool>[] GetTests() method which will return set of tests which will be run by Main function and some of the tests could be mandatory.

And only IsSupported should be implemented directly by every test.

For instance every test method should implement Func[] GetTests() method

Fair enough. We should probably wait and see if moving to use a proper unit testing is viable, however (there may be some actual reason why not for CoreCLR)

4creators · 2018-01-06T15:50:13Z

tests/src/JIT/HardwareIntrinsics/X86/Shared/TestTable.cs

+
+namespace HardwareIntrinsicTest
+{
+    public unsafe struct TestTable<T> : IDisposable where T : struct


TestTable in current form is very deficient for testing more complex operations. As a requirements I would propose:

Support all input data formats for all HW intrinsic operations - Vector128, Vector256, numerical types, an even strings for SSE42.

Store output of intrinsic under test as well as output of verification function alongside test data.

Allow for priniting to output of all collected test data for detailed verification.

Since we may need to have TestTable with 4 or even 5 data arrays it could be necessary to create set of structs or just switch to class and extend it via inheritance.

I have some local work here that I haven't pushed yet.

This was basically renamed to SimpleTestTable and is meant for tests which take expect the same operation to be performed for each input. There are tests in other categories that would require their own new TestTables.

For example, some tests only operate on a single input, some operate on strings or scalar values, and some (like Shuffle or Unpack) require checking inArray1, inArray2, and outArray as a whole, rather than an element at time.

I have some local work as well - I hope I can push it soon. It has all this difficult operations tested with several input - ouput combinations.

4creators · 2018-01-06T16:02:56Z

tests/src/JIT/HardwareIntrinsics/X86/Avx2/Add/UInt64.cs

+
+        private TestTable<ulong> _testTable;
+
+        public Test()


What I find very problematic here is the very limited test scope. Testing single operation on single Vector random elements just scratches the surface. In general Vector operations are fast and it is feasible to run 10s, 100s or even 1000s of them without any significant time penalty.

Furthermore, we should test for all known problematic data points for given data format i.e. for floats we should have predefined problematic data comprising +0.0, -0.0, data close to minimum normal value, subnormal values, 1.0, -1.0 NaN, +infinity and -infinity, set of data around 0.0 and around 1.0 combined into operations with other large randomly generated data set.

Another requirement related to test function would be saving of all input data, output from function under test and output from verification function for later printout when test will fail to provide good diagnostic data.

All tests should run up to the end and only after that errors should be reported for all faulting operations.

Testing single operation on single Vector random elements just scratches the surface.

These are meant to be unit tests, not integration or end-to-end tests. We should be writing other test types (integration, end-to-end, perf, etc) separately and only ensuring that the basic functionality is covered by these tests in particular.

Furthermore, we should test for all known problematic data points for given data format i.e. for floats we should have predefined problematic data comprising

I don't agree.

We are not coding complex algorithms, instead we are taking a method and (in the 95% of cases) generating a single hardware instruction. The vast majority of the complexity comes in the form of containment analysis and ensuring we do the right thing when hardware isn't supported or we are called indirectly.

Provided we are emitting the correct instruction and doing nothing else with the data (which is validated by the basic scenario) we can safely rely on the fact that Intel, AMD, and other hardware manufacturers have likely already done more extensive testing to ensure the underlying hardware instruction is functionally correct.

That beind said, there are few exceptions. These show up in the form of helper functions (Set, SetAll, etc) and for methods which require more extensive codegen (such as CompareEqualUnorderedScalar, which does a comparison, a couple of flag checks, and returns a bool).

Another requirement related to test function would be saving of all input data, output from function under test and output from verification function for later printout when test will fail to provide good diagnostic data.

The way the current code is written, each test will print its failure information (which includes inputs and outputs) and all other tests will continue executing (short of an Exception being thrown or a JIT assert occurring).

we can safely rely on the fact that Intel, AMD, and other hardware manufacturers have likely already done more extensive testing to ensure the underlying hardware instruction is functionally correct

It is not my intention to test function correctness, however, trusting that HW manufacturers are doing their job well is OK but checking it is even better. My intention is to execute all corner cases which may or may not rise exceptions and test how we handle it. I am certain there could be some surprises ahead in there.

have likely already done more extensive testing

This is not a safe assumption to make. You should assume that all testing that was done is using the tests committed in the repo.

You should assume that all testing that was done is using the tests committed in the repo.

My point was that we are providing a contract to the consumer of our API. That contract (for the majority of the hardware intrinsics) is the following:

We will emit a particular instruction

We will pass the inputs, as given

We will return the output, as given

Our contract doesn't (and shouldn't) be giving any guarantees about the correctness of the underlying instruction.

We should be validating our contract (which is what the current tests are doing). Ensuring that special inputs for which the underlying instruction may behave differently is not something that our contract covers, as it is an implementation detail of the instruction we said we were going to emit.

4creators · 2018-01-06T16:09:43Z

tests/src/JIT/HardwareIntrinsics/X86/Avx2/Add/UInt64.cs

+
+namespace HardwareIntrinsicTest
+{
+    public sealed unsafe class Test


I would prefer not to split operation tests into separate test for each operation - data type combination. This would really explode number of projects used to build and run the tests multiplying project files without significant gain in test content. From my checks building tests in coreclr consists from tiny actual code file build time and huge MSBuild overhead. It is even more pronounced during test run, where core logic test run is a minuscule portion of the whole test running time.

Splitting larger tests into tiny ones will only exacerbate these problems.

I would prefer not to split operation tests into separate test for each operation - data type combination. This would really explode number of projects used to build and run the tests multiplying project files without significant gain in test content. From my checks building tests in coreclr consists from tiny actual code file build time and huge MSBuild overhead. It is even more pronounced during test run, where core logic test run is a minuscule portion of the whole test running time.

Ideally we would be using an actual unit test framework (such as xUnit or NUnit), there would be a project per ISA (rather than a project per method or per overload), a class per method or overload, and a Fact per scenario.

However, I've not seen many other tests in mscorlib (outside of perf tests) that support this.

It might be worth changing this however. @jkotas, thoughts?

Unit test frameworks work great for testing framework APIs where it does not really matter how you call the API and it is fine to abstract it away using XUnit theories, etc.

They do not work well for testing codegen where you need the code calling the API to look in a particular way. (Also, the codegen/runtime bugs are pain to debug with unit testing framework loaded in the process since it introduces a lot of noise.)

I think you may want to look at autogenerating the test sources for testing the intrinsics.

tannergooding · 2018-01-06T17:12:57Z

Cherry-picked the fix for the inliner assertion here: #15772

AndyAyersMS · 2018-01-06T17:58:13Z

src/jit/hwintrinsicxarch.cpp

+        // (gtIsRecursiveCall == false) as the gtNewMustThrowException node can mess with the
+        // inliner in unexpected ways.
+
+        if (gtIsRecursiveCall(method))


Getting back to my comment on your last PR, now that we have a use case -- I'd prefer it if this "recursive call ==> must expand" idiom was confined to the importer; you should pass mustExpand down in here to figure out whether bailing out is acceptable behavior.

Will update.

Fixed. Also see #15772, which has the changes standalone.

tannergooding · 2018-01-11T03:36:19Z

Updated to use a T4 text template for generating the tests. Currently just a SimpleBinaryOpTest template which will cover a large number of the intrinsics.

We can add other templates based on test needs.

Working on a change to add JitLog checking to validate the calls are expanded as intrinsics where appropriate, but that can always go in another PR as well.

tannergooding · 2018-01-11T04:40:20Z

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

tannergooding · 2018-01-11T16:51:36Z

The two job failures are unrelated to this change (which only touches HWIntrinsic test code). I've requeued the jobs.

Failure was: baseservices_threading/mutex_misc_waitone2_waitone2

tannergooding · 2018-01-11T18:26:52Z

@fiigii, @jkotas. Do these changes look good to you?

fiigii · 2018-01-11T19:04:29Z

Question. Shall we extract struct TestTable<T> to be shared by all the tests?

tannergooding · 2018-01-11T19:19:53Z

Shall we extract struct TestTable to be shared by all the tests?

I think we can now that I have pulled the validation logic out of it (now it just represents three pinned arrays and should be reusable almost everywhere).

The place where it isn't still usable is for the scalar tests, which read/write a single value from memory.

tannergooding · 2018-01-12T20:13:16Z

FYI. @CarolEidt as well.

4creators · 2018-01-12T22:02:49Z

I would prefer not to split operation tests into separate test for each operation - data type combination. This would really explode number of projects used to build and run the tests multiplying project files without significant gain in test content. From my checks building tests in coreclr consists from tiny actual code file build time and huge MSBuild overhead. It is even more pronounced during test run, where core logic test run is a minuscule portion of the whole test running time.

Splitting larger tests into tiny ones will only exacerbate these problems.

Just digged out important issue which got folded by github

4creators · 2018-01-12T22:08:58Z

Another paleontological discovery hidden by github:

#15771 (comment)

tannergooding · 2018-01-12T22:19:25Z

#15771 (comment)

My response to this is still the same. The hardware intrinsic contract (for all non-helper intrinsics) is that we will:

Throw a PNSE on unsupported hardware
Otherwise, we will:
- emit a particular instruction (using the VEX encoding, on hardware that supports it)
- pass the input (or a reference to the input), as given by the user
- return the output (or a reference to the output), as given back by the instruction

Any particulars about how the underlying hardware operates on any given data is an implementation detail of the hardware instruction and is not something we need to or should be testing.

Provided we are passing in the data unmodified, then it doesn't matter if it is QNaN, SNaN, 0, -0, +Inf, -Inf, denormal, rational, irrational, positive, negative, etc.

tannergooding · 2018-01-12T22:20:18Z

It would be an entirely different story if we were providing a software fallback. But we are not (and even in the case of the compiler fallback, we are still meeting the original contract requirements stated).

fiigii · 2018-01-12T22:24:46Z

This test pattern looks good to me. Is there anybody can approve and merge this PR?

4creators · 2018-01-12T22:27:29Z

Do we care about test efficiency? If yes pls comment on my previous question.

fiigii · 2018-01-12T22:29:58Z

Do we care about test efficiency?

I did not see any problem with the " test efficiency" yet.

tannergooding · 2018-01-12T22:41:58Z

@fiigii, The "issue" @4creators is referring to is the large amount of time it takes to compile all the tests in the framework.

I currently have the tests split out into one project per method instance (so Avx\Add\Single.cs, Avx\Add\Double.cs, Avx2\Add\Byte.cs, etc). This means that there is some additional level of overhead to compile each project and/or to check they are up-to-date.

However, I don't know if this is a real concern. Once tests have been compiled, you can skip building tests again, and when modifying or adding new tests, it is very easy to build just the modified/new projects.

Additionally, I believe this split helps break the tests out into logical, readable, and debuggable components (and makes it much simpler to use with the T4 text templates).

tannergooding · 2018-01-12T22:46:33Z

It might be possible to modify the template to be able to produce multiple outputs, in which case we would be back to a single project per method (i.e. just Avx\Add_r.csproj, Avx\Add_ro.csproj Avx2\Add_r.csproj, etc). We would still have a code file per method instance, but a general Main method which invoked them one at a time.

However, I'm not sure its worth the additional effort to do (and if it is, I think it can be on the backlog of "nice to have").

…template.

…rom a template.

tannergooding · 2018-01-28T01:58:11Z

Ok, new changes are up.

Overall the tests converted are the same, but now it uses the Shared\GenerateTests.csx script and they share a single project file (with a name matching the ISA) rather than one project per test.

tannergooding · 2018-01-28T19:02:06Z

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

fiigii · 2018-01-29T18:37:51Z

but now it uses the Shared\GenerateTests.csx

Thanks for the work. So, is this script executed by the coreclr test build to automatically generate test cases? Or we need to manually run it?

Can we push this PR to get merged as soon as possible? There are more and more intrinsic PRs coming, so we should have a criteriron for HW intrinsic tests to avoid refactoring tests again and again...

tannergooding · 2018-01-29T18:42:19Z

Or we need to manually run it?

At present, it needs to be manually run after you update it with any new input data.

fiigii · 2018-01-29T18:52:01Z

it needs to be manually run after you update it with any new input data.

Good to know. Current test-template looks pretty good, and I think we can merge this PR at first, and extending the template later to support more tests (e.g., JIT-log) is easy.

cc @CarolEidt

sdmaclea · 2018-01-29T18:53:39Z

tests/src/JIT/HardwareIntrinsics/X86/Shared/SimpleBinOpTest.template

+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+


The all generated tests (and therefore the template) should contain a comment noting that the test is generated and should not be manually edited. It should also contain instructions on how to regenerate.

CarolEidt · 2018-01-29T19:45:52Z

@jashook - could you have a look at this? I believe that the use of a single project file per ISA avoids the excessive test build time that I was concerned about, but I'd like your input on this.

This looks good to me overall, but I think there is a need to add some minimal documentation. This is all pretty self-explanatory, I think, but there needs to be a way to guide a JIT/coreclr developer to find the right files. Probably a brief document under https://github.com/dotnet/coreclr/tree/master/Documentation/project-docs?

…ic documentation.

tannergooding · 2018-01-29T23:30:41Z

Updated the generated files to indicate that they are automatically generated from the script and updated the script with basic instructions for how to run or add new tests.

CarolEidt

LGTM - thanks!

CarolEidt · 2018-01-29T23:45:40Z

tests/src/JIT/HardwareIntrinsics/X86/Avx/Add.Double.cs

@@ -2,6 +2,13 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+/******************************************************************************
+ * This file is auto-generated from a template file by the GenerateTests.csx  *
+ * script in tests\src\JIT\HardwareIntrinsics\X86\Shared. In order to make    *


Excellent - great place to put the pointer to the script!

sdmaclea

LGTM

fiigii · 2018-01-30T00:14:54Z

tests/src/JIT/HardwareIntrinsics/X86/Shared/SimpleBinOpTest.template

+            }}
+            else
+            {{
+                for (var i = 1; i < left.Length; i++)


I think we can remove this for loop to enable the template for more intrinsics (e.g., HorizontalAdd).

You mean just check each element individually, rather than looping?

Yes, that would make the template more general.

I'm going to log an issue to track this (as part of porting the remaining tests to use the template as well).

We probably want to refactor it such that we don't have to duplicate a bunch of table data for the regular intrinsics (where each check will be the same).

tannergooding · 2018-01-30T18:53:53Z

Windows_NT x86 Release Innerloop Build and Test passed, but hasn't updated the icon.

CarolEidt · 2018-01-30T21:46:37Z

ping @jashook - do you think you'll find some time to review this soon?

jashook · 2018-01-31T00:04:16Z

@CarolEidt sorry missed this. Taking a look now.

jashook

LGTM. It is worth noting the new tests will have to be added to our lstFile to run them in automation. My guess is that file is quite out of date already.

CarolEidt · 2018-01-31T00:12:35Z

It is worth noting the new tests will have to be added to our lstFile to run them in automation. My guess is that file is quite out of date already.

So, is it the case that newly-added tests are never run in automation? Does that mean ci, or ...?

tannergooding · 2018-01-31T00:17:44Z

Does that mean ci

I'm fairly certain they are being run in CI, it is certainly failing the builds in a number of cases 😄

jashook · 2018-01-31T00:20:09Z

That is the case only for arm/arm64.

tannergooding · 2018-01-31T00:20:48Z

Logged: https://github.com/dotnet/coreclr/issues/16113

CarolEidt · 2018-01-31T00:24:33Z

Thanks @jashook !

jkotas added the area-CodeGen label Jan 6, 2018

4creators reviewed Jan 6, 2018

View reviewed changes

tannergooding mentioned this pull request Jan 6, 2018

Updating the x86 HWIntrinsic importer to not inline when returning gtNewMustThrowException #15772

Merged

AndyAyersMS reviewed Jan 6, 2018

View reviewed changes

tannergooding changed the title ~~[WIP] Update x86 HWIntrinsic Tests~~ Update x86 HWIntrinsic Tests Jan 11, 2018

tannergooding mentioned this pull request Jan 11, 2018

Adding basic containment support to the x86 HWIntrinsics #15804

Merged

tannergooding added 2 commits January 27, 2018 17:56

Updating the Avx2.Add HardwareIntrinsic tests to be generated from a …

ba7172f

…template.

Updating several of the Sse HardwareIntrinsic tests to be generated f…

e40fcda

…rom a template.

sdmaclea suggested changes Jan 29, 2018

View reviewed changes

Updating the auto-generated x86 HWIntrinsic test files to contain bas…

9bfca6a

…ic documentation.

CarolEidt approved these changes Jan 29, 2018

View reviewed changes

CarolEidt requested a review from jashook January 29, 2018 23:47

sdmaclea approved these changes Jan 29, 2018

View reviewed changes

sdmaclea mentioned this pull request Jan 30, 2018

[Arm64] Add Simd HWIntrinsic test #16008

Merged

fiigii reviewed Jan 30, 2018

View reviewed changes

tannergooding mentioned this pull request Jan 30, 2018

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

Merged

jashook approved these changes Jan 31, 2018

View reviewed changes

tannergooding merged commit b47ab22 into dotnet:master Jan 31, 2018

tannergooding mentioned this pull request Jan 31, 2020

Add tests to the lstFile so they can be run in automation. dotnet/runtime#9631

Closed

sdmaclea mentioned this pull request Jan 31, 2020

[Arm64] Script SIMD HW intrinsic test like #15771 dotnet/runtime#9645

Closed

4creators mentioned this pull request Jan 31, 2020

Intel Hardware Intrinsics test framework design deficiences dotnet/runtime#10349

Open

Update x86 HWIntrinsic Tests #15771

Update x86 HWIntrinsic Tests #15771

Conversation

tannergooding commented Jan 6, 2018

tannergooding commented Jan 6, 2018

tannergooding commented Jan 6, 2018 • edited Loading

tannergooding commented Jan 6, 2018

tannergooding commented Jan 6, 2018

4creators Jan 6, 2018 • edited Loading

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

tannergooding commented Jan 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented Jan 11, 2018

tannergooding commented Jan 11, 2018

tannergooding commented Jan 11, 2018

tannergooding commented Jan 11, 2018

fiigii commented Jan 11, 2018

tannergooding commented Jan 11, 2018

tannergooding commented Jan 12, 2018

4creators commented Jan 12, 2018 • edited Loading

4creators commented Jan 12, 2018

tannergooding commented Jan 12, 2018

tannergooding commented Jan 12, 2018

fiigii commented Jan 12, 2018

4creators commented Jan 12, 2018

fiigii commented Jan 12, 2018

tannergooding commented Jan 12, 2018

tannergooding commented Jan 12, 2018

tannergooding commented Jan 28, 2018

tannergooding commented Jan 28, 2018

fiigii commented Jan 29, 2018

tannergooding commented Jan 29, 2018

fiigii commented Jan 29, 2018

Choose a reason for hiding this comment

CarolEidt commented Jan 29, 2018

tannergooding commented Jan 29, 2018

CarolEidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdmaclea 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

tannergooding commented Jan 30, 2018

CarolEidt commented Jan 30, 2018

jashook commented Jan 31, 2018

jashook left a comment

Choose a reason for hiding this comment

CarolEidt commented Jan 31, 2018

tannergooding commented Jan 31, 2018

jashook commented Jan 31, 2018

tannergooding commented Jan 31, 2018

CarolEidt commented Jan 31, 2018

tannergooding commented Jan 6, 2018 •

edited

Loading

4creators Jan 6, 2018 •

edited

Loading

4creators commented Jan 12, 2018 •

edited

Loading