Enable AVX512 Additional 16 SIMD Registers #79544

anthonycanino · 2022-12-12T16:09:56Z

PR enables AVX512 additional 16 SIMD registers for use with the register allocator and codegen emitter. The PR largely consists of the following

Extend xarch regMask type from uint32 to uint64 to make room for additional registers.
Add MM16-MM31 to the register definitions (targetamd.h).
Adjust the RBM macros to refer to globally defined variables (float register availability, calleer/callee save register sets etc.) that can be initialized at runtime (so as to add the upper SIMD registers to the definitions if AVX512 is available). Previously, the bank of registers was entirely determined by the architecture, whereas it is now determined by arch and ISA availability).
Add a new mode, JitStressRegs=4 which forces an upper SIMD register to be selected during register allocation.
Ensure that an upper SIMD register is selected for a gentree node only if the node can be lowered to an instruction that supports EVEX encoding.
Refactor some of the instruction description backend to instead pass around and use an instrDesc struct instead of a pure instruction to determine when an EVEX prefix is needed. Previously, the instruction itself was enough to determine if SSE/VEX is needed --- now, we require more information about how the instruction is used, e.g., does it require an upper SIMD register.
Add logic to emitter to place proper bytes in the EVEX prefix for the upper simd registers.

Of all the points above, (4) requires a little more elaboration. When the LSRA constraints are build up for a node, typically RBM_NONE is passed as the constraint mask for an ref interval, which eventually leads to the LSRA selecting the set of available registers for the ref interval type. For nodes whose lowering does not have an available EVEX form (see the commit message), one would like to specify the constraint mask to RBM_LOWFLOAT or an equivalent set of registers; however, this came into conflict with the way contained nodes are handling during lowering (and comments even state that the constraint mask feature will not work if contained nodes do not have the same type as the containing node).

To mitigate a lot of rewriting, I created a special RBM_LOWSIMD tag in the regMask type which functions much like RBM_NONE except that during the creation of a new ref position (LinearScan::newRefPosition) it will select the low SIMD nodes if the interval is a floating/simd type, otherwise it will default to RBM_NONE functionality.

ghost · 2022-12-12T16:10:10Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

PR is a draft for testing purposes. Will update with a full description of the changes when ready for review.

Author:	anthonycanino
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

kunalspathak · 2022-12-14T07:09:14Z

Probably we need to figure out first how to fix the regressions showing up in windows/x86:

tannergooding · 2022-12-14T16:38:59Z

src/coreclr/jit/emit.h

+ {
+ case IF_RWR_RRD_RRD:
+ case IF_RWR_RRD_RRD_CNS:
+ case IF_RWR_RRD_RRD_RRD:


Is it expected that this one is here and in idHasReg4?

Could we maybe simplify it to default: return idHasReg4()?

src/coreclr/jit/target.h

tannergooding · 2022-12-14T16:46:51Z

src/coreclr/jit/targetamd64.h

@@ -78,10 +78,16 @@
 #endif // !UNIX_AMD64_ABI
 #define CSE_CONSTS 1 // Enable if we want to CSE constants

- #define RBM_ALLFLOAT (RBM_XMM0 | RBM_XMM1 | RBM_XMM2 | RBM_XMM3 | RBM_XMM4 | RBM_XMM5 | RBM_XMM6 | RBM_XMM7 | RBM_XMM8 | RBM_XMM9 | RBM_XMM10 | RBM_XMM11 | RBM_XMM12 | RBM_XMM13 | RBM_XMM14 | RBM_XMM15)
+ #define RBM_LOWFLOAT (RBM_XMM0 | RBM_XMM1 | RBM_XMM2 | RBM_XMM3 | RBM_XMM4 | RBM_XMM5 | RBM_XMM6 | RBM_XMM7 | RBM_XMM8 | RBM_XMM9 | RBM_XMM10 | RBM_XMM11 | RBM_XMM12 | RBM_XMM13 | RBM_XMM14 | RBM_XMM15 )
+ #define RBM_HIGHFLOAT (RBM_XMM16 | RBM_XMM17 | RBM_XMM18 | RBM_XMM19 | RBM_XMM20 | RBM_XMM21 | RBM_XMM22 | RBM_XMM23 | RBM_XMM24 | RBM_XMM25 | RBM_XMM26 | RBM_XMM27 | RBM_XMM28 | RBM_XMM29 | RBM_XMM30 | RBM_XMM31)


I wonder if we could come up with a better name than "HIGHFLOAT"

From the initial 8 SIMD registers, we've had 2 extensions so far and its conceivable a 3rd could happen in the future

I've used "high" to refer to the 16 AVX512 registers mostly because that is the term reference in the Intel manual, with "next" being used to reference the 8 AVX registers.

I'm in favor of a better naming scheme, but not certain what to choose. We could select RBM_AVX512FLOAT or something along those lines...

@tannergooding any more thoughts on this?

I don't have a particular preference and I don't think it's "strictly" necessary to update. More a passing thought about how it might be confusing.

anthonycanino · 2022-12-14T18:06:19Z

Probably we need to figure out first how to fix the regressions showing up in windows/x86:

I am investigating why we see a throughput hit on x86, it wasn't quite what I thought it was earlier.

BruceForstall · 2022-12-14T18:15:34Z

@anthonycanino Looks like you need to fix the formatting.

Diffs -- no diffs but reasonably significant TP regressions (~0.5%) on x64.

BruceForstall · 2022-12-14T18:17:36Z

Do you want to include https:/dotnet/runtime/compare/main...BruceForstall:EnableJitStressHighAvx512Regs?expand=1 in this change?

anthonycanino · 2022-12-14T18:24:20Z

Do you want to include https:/dotnet/runtime/compare/main...BruceForstall:EnableJitStressHighAvx512Regs?expand=1 in this change?

Yes I think that would be a good idea. Do I merge it in this this PR or do we do that seperately?

BruceForstall · 2022-12-14T18:30:34Z

Yes I think that would be a good idea. Do I merge it in this this PR or do we do that seperately?

Include the changes as a new commit. Doesn't need to be a branch merge from mine or anything like that.

BruceForstall · 2022-12-14T19:15:40Z

Looks like this is likely to conflict with #79478

anthonycanino · 2022-12-14T21:11:52Z

Looks like this is likely to conflict with #79478

I've rebased and pushed to test now.

src/coreclr/jit/compiler.h

src/coreclr/jit/compiler.cpp

BruceForstall · 2022-12-14T20:13:04Z

src/coreclr/jit/compiler.cpp

+#if defined(TARGET_AMD64)
+ rbmAllFloat = RBM_ALLFLOAT_INIT;
+ rbmFltCalleeTrash = RBM_FLT_CALLEE_TRASH_INIT;
+ rbmCntCalleeTrashFloat = CNT_CALLEE_TRASH_FLOAT_INIT;
+
+ if (DoJitStressEvexEncoding())
+ {
+ rbmAllFloat |= RBM_HIGHFLOAT;
+ rbmFltCalleeTrash |= RBM_HIGHFLOAT;
+ rbmCntCalleeTrashFloat += 16;
+ }
+ rbmCalleeTrash = RBM_CALLEE_TRASH_INIT;
+ rbmCalleeTrashNoGC = RBM_CALLEE_TRASH_NOGC_INIT;
+ rbmCalleeTrashWriteBarrier = RBM_CALLEE_TRASH_WRITEBARRIER_INIT;
+ rbmCalleeGCTrashWriteBarrier = RBM_CALLEE_GCTRASH_WRITEBARRIER_INIT;
+ rbmCalleeTrashWriteBarrierByref = RBM_CALLEE_TRASH_WRITEBARRIER_BYREF_INIT;
+ rbmCalleeGCTrashWriteBarrierByref = RBM_CALLEE_GCTRASH_WRITEBARRIER_BYREF_INIT;
+ rbmStopForGCTrash = RBM_STOP_FOR_GC_TRASH_INIT;
+ rbmInitPInvokeFrameTrash = RBM_INIT_PINVOKE_FRAME_TRASH_INIT;
+ rbmProfilerEnterTrash = RBM_PROFILER_ENTER_TRASH_INIT;
+ rbmProfilerLeaveTrash = RBM_PROFILER_LEAVE_TRASH_INIT;
+ rbmProfilerTailcallTrash = RBM_PROFILER_TAILCALL_TRASH_INIT;
+#endif // TARGET_AMD64


So these are global variables now, that are initialized every time the JIT is initialized?

Can we assume the memory write is atomic? Should they be initialized using InterlockedExchange64? Using Interlocked, we could assert they either have the same value as written, or zero (uninitialized).

I presume they are not on Compiler because either (1) it was inconvenient to plumb a Compiler* everywhere they are needed, (2) throughput, or (3) you couldn't/didn't want to use the TLS Compiler* (maybe only available in DEBUG?)

Could there be a scenario where these change for a single clrjit.dll instance? E.g., what if crossgen2/AOT compilation loads the JIT and compiles both an AVX-512 targeting function and a non-AVX-512 targeting function?

Currently, they are only changed with the JitStress mode. They will presumably be set based on ISA. Does this initialization location have all the appropriate ISA information?

So these are global variables now, that are initialized every time the JIT is initialized?

Can we assume the memory write is atomic? Should they be initialized using InterlockedExchange64? Using Interlocked, we could assert they either have the same value as written, or zero (uninitialized).

I presume they are not on Compiler because either (1) it was inconvenient to plumb a Compiler* everywhere they are needed, (2) throughput, or (3) you couldn't/didn't want to use the TLS Compiler* (maybe only available in DEBUG?)

Yes, this is why --- passing compiler around was both clunky and also incurred its own overhead because it needed to be checked on each access.

Could there be a scenario where these change for a single clrjit.dll instance? E.g., what if crossgen2/AOT compilation loads the JIT and compiles both an AVX-512 targeting function and a non-AVX-512 targeting function?

I could use some help here, because honestly I'm not familiar enough with the internals to understand this. As it's done now, I am assuming that these are unchanged and compiling for a single arch/ISA.

Currently, they are only changed with the JitStress mode. They will presumably be set based on ISA. Does this initialization location have all the appropriate ISA information?

The compiler object has this information as far as I know. Essentially, I would envision replacing with an compOpportunisticallyDependsOn(InstructionSet_AVX512F_VL) check.

@tannergooding @kunalspathak are my assumptions here correct? Will this be an issue with crossgen?

I think it needs to be filtered oncompOpportunisticallyDependsOn(InstructionSet_AVX512F), but I agree that I would expect crossgen to be work correctly presuming that.

I do think this needs to be "per compiler instance" and not part of the global state as we can have AltJits or other scenarios where one compiler supports an ISA but another does not.

If we could cheapen this further, such as pulling the relevant state into a local where the register allocator needs it, that would presumably be "better".

If the TP regression is "too much" even with that, I could imagine a few ways we could use a template or inlining to ensure the LSRA code still uses a constant and is primarily filtered from a centralized high level check (of course, this comes at the cost of increased deployment size for clrjit.dll).

If we could cheapen this further, such as pulling the relevant state into a local where the register allocator needs it

The problem is it is not just LSRA that depends on them, but optimizer and codegen as well. Hence it is little hard to extract that out just at the LSRA level.

I presume they are not on Compiler because either (1) it was inconvenient to plumb a Compiler* everywhere they are needed, (2) throughput, or (3) you couldn't/didn't want to use the TLS Compiler* (maybe only available in DEBUG?)

The reason was #1 , but what you have brought up is a legitimate concern. The only way to avoid it to have them on compiler object and then pass compiler instance around. Depending on which area are using these macros, we could do something like, where you have a macro (per file since they are not that many) which takes compiler object, but at all the places where it is needed, you use a version of macro that actually call the one that takes compiler object. That way, at the callsite, you don't have to pass compiler object. Its ugly :(

#define ABC_WITH_COMPILER(compiler) compiler->isEvex ? x : y #define ABC ABC_WITH_COMPILER(this) = ABC;

@kunalspathak @tannergooding do you have any more thoughts on how to adjust the PR for this?

Did you try the approach in #79544 (comment) and #79544 (comment)?

Ok, let me try this approach #79544 (comment) then and see how the numbers look.

@kunalspathak I have a draft of this up at 9c386bc . Currently waiting for test data / throughput.

To be honest, I do not like it, it seems really messy (even if we move the macro definitions to the call sites). It also seems the alternatives aren't great either.

FYI, with the change 9c386bc ...

It seems like most approaches have been pretty similar with throughput impact.

src/coreclr/jit/compiler.cpp

src/coreclr/jit/hwintrinsic.h

src/coreclr/jit/target.h

src/coreclr/jit/emitxarch.cpp

BruceForstall · 2022-12-15T00:38:16Z

Triggered avx-512 pipeline: https://dev.azure.com/dnceng-public/public/_build/results?buildId=112900&view=results

kunalspathak

Added some comments for code around LSRA.

kunalspathak · 2022-12-14T19:36:53Z

src/coreclr/jit/emitinl.h

@@ -214,7 +214,8 @@ inline ssize_t emitter::emitGetInsAmdAny(instrDesc* id)

 /*static*/ inline void emitter::emitEncodeCallGCregs(regMaskTP regmask, instrDesc* id)
 {
- assert((regmask & RBM_CALLEE_TRASH) == 0);
+ // TODO-XARCH-AVX512 global defined in compiler.h, not in scope here


fix and uncomment?

src/coreclr/jit/targetamd64.h

kunalspathak · 2022-12-15T02:24:46Z

src/coreclr/jit/lsraxarch.cpp

@@ -1900,21 +1905,30 @@ int LinearScan::BuildIntrinsic(GenTree* tree)
 break;
 }
 assert(tree->gtGetOp2IfPresent() == nullptr);
+
+// TODO-XARCH-AVX512 this is overly constraining register available as NI_System_Math_Abs


Can we have this extracted out in a separate method (along with the comment) instead of having this check at multiple places?

Also, shouldn't this be under isEvexCompatible check instead of just using RBM_LOWSIMD for AMD64?

In this case, its the NamedIntrinsics and not the HWIntrinsics, e.g., those defined in namedintrinsiclist.h.

I suppose I can try and add a similar marking for these but as far as I know there is not a set of flags for these named intrinsics?

kunalspathak · 2022-12-15T02:28:38Z

src/coreclr/jit/lsraxarch.cpp

- srcCount += isRMW ? BuildDelayFreeUses(op3, op1) : BuildOperandUses(op3);
+ regMaskTP op3RegCandidates = RBM_NONE;
+#if defined(TARGET_AMD64)
+ if (!isEvexCompatible)


This is how I would expect the above code too.

kunalspathak · 2022-12-15T02:31:20Z

src/coreclr/jit/lsrabuild.cpp

+ if (mask == RBM_LOWSIMD)
+ {
+ // Constrain if we have to for float/simd types
+ if (varTypeIsFloating(theInterval->registerType) || varTypeIsSIMD(theInterval->registerType))


This is heavy check to add in newRefPosition. Is there a way to move this check (you can have this logic in a helper method or something) to relevant callers which would definitely pass mask == RBM_LOWSIMD?

At the callsite, since you already have the GenTree* for the RefPosition, you can do something like this:

regMaskTP mask = RBM_NONE; if (varTypeIsFloat(node) || varTypeIsSIMD(node)) { mask = lowSIMDRegs(); } BuildUse(node, mask);

I believe there was a case where specifying the mask at a BuildUse callsite did not cover, something related to a contained GT_INDIR node whose register set was not the same as its containing node.

Let me see if I can find the example.

With this change 6cd4d43 we can see the following example cause some potential issues...

[MethodImpl(MethodImplOptions.NoInlining)] static bool Test_Sse41_TestZ_Normal(in Vector128<Int32> x, in Vector128<Int32> y) { return Sse41.TestZ(x, y); }

which lowers to....

N001 ( 1, 1) [000000] ----------- t0 = LCL_VAR byref V00 arg0 u:1 (last use) $80 /--* t0 byref N002 ( 7, 5) [000001] ---XG------ t1 = * IND simd16 N003 ( 1, 1) [000002] ----------- t2 = LCL_VAR byref V01 arg1 u:1 (last use) $81 /--* t2 byref N004 ( 7, 5) [000003] -c-XG------ t3 = * IND simd16 /--* t1 simd16 +--* t3 simd16 N005 ( 15, 11) [000004] ---XG------ * HWINTRINSIC void int PTEST <l:$243, c:$242>

Since TestZ associated lowered instruction ptest does not have an EVEX form, we restrict it's operands (which are both float variables that are contained) to the lower SIMD registers. I believe this creates an issue though since if we specify constraints on t3 = IND it also propagates to its child node t2 = LCL_VAR byref .... but the nodes have different variable types. Previously, RBM_NONE would have been passed down, which would let newRefPosition select a set of registers based on ref positions type.

Does this make sense? Essentially, RBM_LOWSIMD was a version of RBM_NONE to get around this issue.

Hhm, I thought you were planning to remove it. Did you forget to push?

kunalspathak · 2023-02-06T21:44:29Z

Latest TP numbers:

anthonycanino · 2023-02-07T17:47:53Z

@dotnet/avx512-contrib can I get a rerun of the avx512 stress pipelines?

tannergooding · 2023-02-07T17:52:12Z

Requeued the failing jobs.

anthonycanino · 2023-02-07T18:12:24Z

Latest TP numbers:

What are our thoughts on this? Are we in acceptable range?

tannergooding · 2023-02-07T18:19:30Z

-- Just worth noting that between November and now we've gotten multiply TP improvements in, including some that were -0.4% range, so we likely won't be regressing anything as compared to .NET 7

There are likely more TP improvements to be had for this code of course, but it's potentially something we can iterate on separately.

kunalspathak · 2023-02-07T19:13:53Z

but it's potentially something we can iterate on separately.

I agree.

anthonycanino · 2023-02-07T22:23:08Z

So are we ok with the changes now?

BruceForstall · 2023-02-08T01:36:12Z

Can you remind me why there is any TP impact on x86?

[Edit] I suppose that could be due to the instruction ins => const instrDesc* id parameter change, for example.

anthonycanino · 2023-02-08T01:47:16Z

Can you remind me why there is any TP impact on x86?

[Edit] I suppose that could be due to the instruction ins => const instrDesc* id parameter change, for example.

Yes, this is what I think.

BruceForstall · 2023-02-08T02:45:38Z

It looks like on Linux (and Mac?) we'll need additional updates in src\coreclr\pal\src\thread\context.cpp, specifically CONTEXTFromNativeContext, CONTEXTToNativeContext, as well as FPREG_HasYmmRegisters() (need a FPREG_HasZmmRegisters()?) and audit other places where CONTEXT_XSTATE is used and is storing 16 registers, 16 bytes each. So it looks like they would need updating to save/restore kmask, the high 16 bytes of zmm0-zmm15 and all of zmm16-zmm31.

But maybe that's not causing problems in this change, and can be deferred to a separate change?

cc @janvorli @DeepakRajendrakumaran

Convert from macros to accessor functions for RBM_ALLFLOAT, RBM_FLT_CALLEE_TRASH, CNT_CALLEE_TRASH_FLOAT. Convert LSRA use of ACTUAL_REG_COUNT to AVAILABLE_REG_COUNT, and create an accessor for that value for AMD64 as well.

BruceForstall · 2023-02-08T07:11:24Z

The macros defining the AVX512-specific masks and counts were bothering me, so I wrote #81818, to implement them as accessor functions in the various classes that need them. I think it looks better this way. Comments?

[Edit] I created a PR to this overall change if you decide to take it: anthonycanino#6

kunalspathak

Register allocator portion looks good to me modulo the changes suggested by @BruceForstall in #81818.

kunalspathak · 2023-02-08T07:44:02Z

src/coreclr/jit/lsrabuild.cpp

-const unsigned lsraRegOrderSize = ArrLen(lsraRegOrder);
+static const regNumber lsraRegOrder[] = {REG_VAR_ORDER};
+const unsigned lsraRegOrderSize = ArrLen(lsraRegOrder);
+// TODO-XARCH-AVX512 we might want to move this to be configured with the rbm variables too


Should this happen in this PR?

Just noting will do in a follow up PR

src/coreclr/jit/lsraxarch.cpp

anthonycanino · 2023-02-08T18:23:03Z

The macros defining the AVX512-specific masks and counts were bothering me, so I wrote #81818, to implement them as accessor functions in the various classes that need them. I think it looks better this way. Comments?

[Edit] I created a PR to this overall change if you decide to take it: anthonycanino#6

Yes, I will pull in.

…egs-with-reg-accessors Use inline accessor functions instead of macros

BruceForstall · 2023-02-08T23:06:52Z

No diffs, TP regressions as before.

Failures looks like infra or unrelated.

kunalspathak

LGTM. Thanks for your patience and efforts!

anthonycanino · 2023-02-08T23:36:08Z

Thanks all!

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 12, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 12, 2022

build-analysis bot mentioned this pull request Dec 12, 2022

Precondition failure: File has not had execution verified #79439

Closed

teo-tsirpanis added the avx512 Related to the AVX-512 architecture label Dec 12, 2022

anthonycanino force-pushed the avx512-upper-regs branch from d169ba4 to 317d180 Compare December 13, 2022 22:02

build-analysis bot mentioned this pull request Dec 14, 2022

Build fails with "eng/common/tools.sh: line 474: 537 Segmentation fault" #76759

Closed

tannergooding reviewed Dec 14, 2022

View reviewed changes

src/coreclr/jit/target.h Outdated Show resolved Hide resolved

tannergooding reviewed Dec 14, 2022

View reviewed changes

anthonycanino marked this pull request as ready for review December 14, 2022 18:05

BruceForstall requested review from tannergooding, BruceForstall and kunalspathak December 14, 2022 18:15

anthonycanino mentioned this pull request Dec 14, 2022

[Draft] Isolating instruction to instrDesc refactor changes. #79670

Closed

anthonycanino force-pushed the avx512-upper-regs branch from c2ee20b to 4011e25 Compare December 14, 2022 21:10

BruceForstall requested changes Dec 15, 2022

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 15, 2022

This was referenced Dec 15, 2022

Tracking issue for CI build timeouts #76454

Closed

[wasm] Library tests failing during linking for AOT - SIGKILL #79569

Closed

kunalspathak requested changes Dec 15, 2022

View reviewed changes

tannergooding mentioned this pull request Feb 6, 2023

Have gen* methods use the optimized BitOperations where possible #81697

Closed

Adjust ACTUAL_REG_COUNT based on availability of AVX512.

5490617

Use inline accessor functions instead of macros

3c7acdb

Convert from macros to accessor functions for RBM_ALLFLOAT, RBM_FLT_CALLEE_TRASH, CNT_CALLEE_TRASH_FLOAT. Convert LSRA use of ACTUAL_REG_COUNT to AVAILABLE_REG_COUNT, and create an accessor for that value for AMD64 as well.

BruceForstall mentioned this pull request Feb 8, 2023

Enable AVX512 Additional 16 SIMD Registers, with accessors instead of macros #81818

Closed

kunalspathak reviewed Feb 8, 2023

View reviewed changes

anthonycanino added 2 commits February 8, 2023 10:23

Merge pull request #6 from BruceForstall/anthonycanino_avx512-upper-r…

e42db99

…egs-with-reg-accessors Use inline accessor functions instead of macros

Clearifying comments.

10b4703

This was referenced Feb 8, 2023

Update Linux/Mac Context State for AVX512 #81846

Closed

Optimize lsraRegOrder and buildPhysRegRecords to skip non-AVX512 registers if AVX512 not available #81847

Closed

BruceForstall approved these changes Feb 8, 2023

View reviewed changes

kunalspathak approved these changes Feb 8, 2023

View reviewed changes

kunalspathak merged commit ad52afd into dotnet:main Feb 8, 2023

build-analysis bot mentioned this pull request Feb 9, 2023

[wasm] Runtime test simpleprovidervalidation.sh failing on CI #81859

Closed

BruceForstall mentioned this pull request Feb 9, 2023

Cache copies of register sets and data that varies with AVX512 #81868

Merged

kunalspathak mentioned this pull request Mar 7, 2023

Segregate the register iterations based on intType/floatType #83109

Closed

2 tasks

ghost locked as resolved and limited conversation to collaborators Mar 11, 2023

Enable AVX512 Additional 16 SIMD Registers #79544

Enable AVX512 Additional 16 SIMD Registers #79544

Conversation

anthonycanino commented Dec 12, 2022 • edited Loading

ghost commented Dec 12, 2022

kunalspathak commented Dec 14, 2022

tannergooding Dec 14, 2022 • 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

anthonycanino commented Dec 14, 2022

BruceForstall commented Dec 14, 2022

BruceForstall commented Dec 14, 2022

anthonycanino commented Dec 14, 2022

BruceForstall commented Dec 14, 2022

BruceForstall commented Dec 14, 2022

anthonycanino commented Dec 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding Jan 6, 2023 • 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

BruceForstall commented Dec 15, 2022

kunalspathak 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalspathak commented Feb 6, 2023

anthonycanino commented Feb 7, 2023

tannergooding commented Feb 7, 2023

anthonycanino commented Feb 7, 2023

tannergooding commented Feb 7, 2023 • edited Loading

kunalspathak commented Feb 7, 2023

anthonycanino commented Feb 7, 2023

BruceForstall commented Feb 8, 2023 • edited Loading

anthonycanino commented Feb 8, 2023

BruceForstall commented Feb 8, 2023

BruceForstall commented Feb 8, 2023 • edited Loading

kunalspathak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonycanino commented Feb 8, 2023

BruceForstall commented Feb 8, 2023

kunalspathak left a comment

Choose a reason for hiding this comment

anthonycanino commented Feb 8, 2023

anthonycanino commented Dec 12, 2022 •

edited

Loading

tannergooding Dec 14, 2022 •

edited

Loading

tannergooding Jan 6, 2023 •

edited

Loading

tannergooding commented Feb 7, 2023 •

edited

Loading

BruceForstall commented Feb 8, 2023 •

edited

Loading

BruceForstall commented Feb 8, 2023 •

edited

Loading