Enable devirtualization in more scenarios on crossgen2 #53567

davidwrighton · 2021-06-02T01:44:41Z

Address deficiencies in current devirtualization infrastructure

Remove the responsibility of creating a CORINFO_RESOLVED_TOKEN structure from the JIT and make it a responsibility of the VM side of the jit interface.
- This enables the component (crossgen2) which has deeper understanding of the requirements here to correctly handle scenarios that would otherwise require expressing crossgen2 specific details across the jit interface.
Add a new set of fixups (READYTORUN_FIXUP_Check_VirtualFunctionOverride and READYTORUN_FIXUP_Verify_VirtualFunctionOverride) these are used to validate that the behavior of the runtime and crossgen2 compiler is equivalent for a virtual resolution event
- READYTORUN_FIXUP_Check_VirtualFunctionOverride will ensure that the virtual resolution decision is the same at crossgen2 time and runtime, and if the decision differs, any generated code affected by the decision will not be used.
- READYTORUN_FIXUP_Verify_VirtualFunctionOverride will perform the same checks as READYTORUN_FIXUP_Check_VirtualFunctionOverride, but if it fails the check, the process will be terminated with a fail-fast. It is intended for use under the --verify-type-and-field-layout stress mode.
- Currently only the READYTORUN_FIXUP_Verify_VirtualFunctionOverride is actually generated, and it is only generated when using the --verify-type-and-field-layout switch to crossgen2. Future work will identify if there are scenarios where we need to generate the READYTORUN_FIXUP_Check_VirtualFunctionOverride flag. One area of possible concern is around covariant returns, another is around handling of type equivalence.
In order to express the fixup signature for the VirtualFunctionOverride fixups, a new flag has been added to ReadyToRunMethodSigFlags. READYTORUN_METHOD_SIG_UpdateContext will allow the method signature to internally specify the assembly which is associated with the method token, instead of relying on the ambient context.
R2RDump and the ReadyToRun format documentation have been updated with the details of the new fixups/flags.
Update the rules for handling unboxing stubs
- See Don't forget to pass instantiation context after devirtualizing #51918 for details. This adds a new test, as well as proper handling for unboxing stubs to match the JIT behavior
- Also revert JIT: work around issues invoking the unboxed entry when prejitting #52605, which avoided the problem by simply disabling devirtualization in the presence of structs
Adjust the rules for when it is legal to devirtualize and maintain version resiliency
- The VersionsWithCode and VersionsWithType rules are unnecessarily restrictive.
- Instead Validate that the metadata is safely checkable, and rely on the canInline logic to ensure that no IL that can't be handled is inlined.
- This also involved adding a check that the chain of types from the implementation type to the declaration method table type is within the version bubble.
- And changing the VersionsWithType check on the implementation type, to a new VersionsWithTypeReference check which can be used to validate that the type can be referred to, in combination with using VersionsWithType on the type definition.
- By adjusting the way that the declMethod is referred to, it becomes possible to use the declMethod without checking the full method is VersionsWithCode, and it just needs a relationship to version matching code.
In resolveVirtualMethod generate the CORINFO_RESOLVED_TOKEN structures for the jit
- In particular we are now able to resolve to methods where the decl method is the resolution result but is not within the version bubble itself. This can happen if we can prove that the decl method is the only method which can possibly implement a virtual.
Add support for devirtualization reasons to crossgen2
Port all devirtualization abort conditions to crossgen2 from runtime that were not already present
Fix devirtualization from a canonical virtual method when the actual implementation is more exact
Fix variant interface override scenario where there is an interface that requires implementation of the variant interface as well as the variant interface itself.

This is a difference between CoreCLR and the managed type system - the result of interface method resolution is going to be a canonical method on the CoreCLR side, but exact on the managed side. So this implementation is slightly different from how it looks like on the VM side. We slightly touched on the difference in dotnet#45744 but there was more to it.

… scenarios in which it is used

…ion override behavior - Enable use of these fixups for devirtualization scenarios - when --verify-type-and-field-layout is specified, the verification variant of the fixup is used - Update documentation and r2rdump utility

…that we won't devirtualize outside of the version bubble

… ok though, as I think type forwarders can make this R2R unsafe

…info

…mentation and instantiated tables as the owner type is sufficient for now. - Also, set resolvedTokenDevirtualizedUnboxedMethod correctly

Use correct token

…e type as a MethodImpl override

…eferred to

… it is used in such a way

… been determined via analysis

…pe signature generation, use it in the InstrumentationDataTableNode

…de on non-locally defined interfaces. - decl vs impl check done on exact result, not on canonicalized result

…e are implemented in crossgen2 - Provide devirtualization details for all possible ways devirt can fail in crossgen2 (This is more detailed than the crossgen output)

davidwrighton · 2021-06-09T15:51:06Z

@dotnet/crossgen-contrib

AndyAyersMS

Looked over the jit changes, which generally look good, and skimmed the rest, much of which I'm not going to be able to review effectively.

This will need a new jit guid.

There will need to be some updates to superpmi's handling of resolveVirtualMethod; you can likely use the existing helpers for resolved tokens (eg SpmiRecordsHelper::StoreAgnostic_CORINFO_RESOLVED_TOKENout) Seems like you might need a novel combo of the in and out helpers though, as in this case all the fields of the resolved token are outputs...?

AndyAyersMS · 2021-06-09T16:00:31Z

src/coreclr/jit/fginline.cpp

@@ -731,7 +731,7 @@ Compiler::fgWalkResult Compiler::fgLateDevirtualization(GenTree** pTree, fgWalkD
            CORINFO_CONTEXT_HANDLE context                = nullptr;
            const bool             isLateDevirtualization = true;
            bool explicitTailCall = (call->AsCall()->gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0;
-            comp->impDevirtualizeCall(call, &method, &methodFlags, &context, nullptr, isLateDevirtualization,
+            comp->impDevirtualizeCall(call, NULL, &method, &methodFlags, &context, nullptr, isLateDevirtualization,


Suggested change

comp->impDevirtualizeCall(call, NULL, &method, &methodFlags, &context, nullptr, isLateDevirtualization,

comp->impDevirtualizeCall(call, nullptr, &method, &methodFlags, &context, nullptr, isLateDevirtualization,

AndyAyersMS · 2021-06-09T16:01:01Z

src/coreclr/jit/importer.cpp

@@ -20683,6 +20684,7 @@ bool Compiler::IsMathIntrinsic(GenTree* tree)
 //     is not exactly known, and there is a plausible guess for the type.
 //
 void Compiler::impDevirtualizeCall(GenTreeCall*            call,
+                                   CORINFO_RESOLVED_TOKEN* pResolvedToken,


Can you add a comment for the new arg to the block just above?

AndyAyersMS · 2021-06-09T16:02:37Z

src/coreclr/jit/importer.cpp

+    dvInfo.objClass                    = likelyClass;
+    dvInfo.context                     = *pContextHandle;
+    dvInfo.exactContext                = *pContextHandle;
+    dvInfo.pResolvedTokenVirtualMethod = NULL;


Suggested change

dvInfo.pResolvedTokenVirtualMethod = NULL;

dvInfo.pResolvedTokenVirtualMethod = nullptr;

AndyAyersMS · 2021-06-09T16:07:10Z

.../aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DevirtualizationManager.cs

+            //    jit interface logic after the logic that executes here.
+            //
+            // ImplType checking
+            // 1. At all times the metadata definition of the implmenetation type must version with the application.


Suggested change

// 1. At all times the metadata definition of the implmenetation type must version with the application.

// 1. At all times the metadata definition of the implementation type must version with the application.

trylek

Looks beautiful to me, thank you!

trylek · 2021-06-09T19:34:34Z

...ompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/VirtualResolutionFixupSignature.cs

+            sb.Append(":");
+            sb.Append(nameMangler.GetMangledTypeName(_implType));
+            sb.Append(":");
+            _implMethod.AppendMangledName(nameMangler, sb);


Should we apply a null check before calling _implMethod.AppendMangledName?

trylek · 2021-06-09T19:35:23Z

...ompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/VirtualResolutionFixupSignature.cs

+            if (result != 0)
+                return result;
+
+            return _implMethod.CompareTo(otherNode._implMethod, comparer);


dtto - null check on _implMethod

In fact, one more fun here that _implMethod and otherNode._implMethod being both null is probably a valid match.

trylek · 2021-06-09T19:38:22Z

.../tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs

+
+            public bool Equals(VirtualResolutionFixupSignatureFixupKey other)
+            {
+                return FixupKind == other.FixupKind && DeclMethod.Equals(other.DeclMethod) && ImplType == other.ImplType && ImplMethod.Equals(other.ImplMethod);


Do we need the null check here as well?

trylek · 2021-06-09T19:39:22Z

...r/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs

@@ -135,6 +135,11 @@ private void CreateNodeCaches()
                );
            });

+            _virtualFunctionOverrideCache = new NodeCache<VirtualResolutionFixupSignature, ISymbolNode>(key =>
+            {
+                return new PrecodeHelperImport(_codegenNodeFactory,key);


Nit - missing space after the comma

trylek · 2021-06-09T19:44:55Z

src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs

@@ -399,6 +416,93 @@ private bool ComputeTypeVersionsWithCode(TypeDesc type)
            return ComputeInstantiationVersionsWithCode(type.Instantiation, type);
        }

+        private bool ComputeTypeReferenceVersionsWithCode(TypeDesc type)
+        {
+            switch(type.Category)


Could we use type.IsPrimitive here like we do below in ComputeInstantiationVersionsWithCode? In fact, if this smallest set of "absolutely non-versionable types" means the same thing in both methods, could we consider making a helper method for it, something like IsPrimitiveObjectOrString?

Its not quite the same as ComputeInstantiationVersionsWithCode. This one includes Void, but I've used the IsPrimitive and IsVoid predicates, and added a comment.

trylek · 2021-06-09T19:47:03Z

src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs

@@ -247,6 +247,24 @@ public void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb)
                sb.Append("; UNBOXING");
        }

+        public override string ToString()
+        {
+            StringBuilder debuggingName = new StringBuilder();


Could we implement ToString by just calling AppendMangledName on a local Utf8StringBuilder and returning its ToString - i.o.w. is there any fundamental reason for the duplication?

Its not quite the same, in particular, ToString() doesn't have access to a NameMangler.

trylek · 2021-06-09T19:52:19Z

src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunSignature.cs

+                    if (fixupType == ReadyToRunFixupKind.Check_TypeLayout)
+                        builder.Append(" (Check_VirtualFunctionOverride)");
+                    else
+                        builder.Append(" (Verify_VirtualFunctionOverride)");


Nit - today R2RDump uses the "macro" casing for fixup type names. I don't have a strong opinion about this, perhaps it's better to stick with the constant names but at some point we should probably convert the rest to be consistent.

trylek · 2021-06-09T19:53:34Z

src/coreclr/vm/jitinterface.cpp

+                    }
+
+                    SString fatalErrorString;
+                    fatalErrorString.Printf(W("Verify_VirtualFunctionOverride Decl Method '%s' on type '%s' is '%s'(actual) instead of expected '%s'(from compiler)"),


Beautiful, thank you for providing the rich diagnostic info upon failure, that's super useful for analyzing lab failures!

trylek · 2021-06-09T19:55:52Z

src/coreclr/vm/zapsig.cpp

+        uint32_t updatedModuleIndex;
+        IfFailThrow(sig.GetData(&updatedModuleIndex));
+#ifdef FEATURE_MULTICOREJIT
+        if (pZapSigContext->externalTokens == ZapSig::MulticoreJitTokens)


Just for my education - what is this used for? Is that something we should take into account in Crossgen2 itself?

MulticoreJitTokens is used for the MulticoreJit feature, which is an odd profile guided optimization feature that works on a local machine. We don't need to worry about it in crossgen2.

trylek · 2021-06-09T20:00:30Z

src/tests/JIT/opt/Devirtualization/GitHub_51918.cs

+    public int _v1;
+    public int _v2;
+
+    public GenericClass()


I can find the original issue but it would be extremely helpful if you could add at least a three-line comment describing the way the test is supposed to work and how it detects bad behavior - in my recent work on fixing the various regression tests with Crossgen2 I've always had to spend quite a bit of time poring over the particular test to understand what it's supposed to be doing especially when it's exercising some subtle behavioral differences.

- Fix variant interface dispatch bug where the variant interface is implemented by another interface associated with the type. - Handle default interface methods - Except for variant dispatch, detect those, and fail the devirtualization

MichalStrehovsky · 2021-06-10T07:50:14Z

src/coreclr/tools/Common/TypeSystem/IL/MethodIL.cs

    {
        /// <summary>
-        /// Gets the method whose body this <see cref="MethodIL"/> represents.
+        /// Gets the method whose body this <see cref=""/> represents.


Suggested change

/// Gets the method whose body this <see cref=""/> represents.

/// Gets the method whose body this <see cref="MethodILScope"/> represents.

MichalStrehovsky · 2021-06-10T07:53:41Z

src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs

 using Internal.TypeSystem;
-
+using CORINFO_DEVIRTUALIZATION_DETAIL = Internal.JitInterface.CORINFO_DEVIRTUALIZATION_DETAIL;


This is technically a layering violation since DevirtualizationManager is the general purpose component that can be used with different codegen backends.

But I don't know if I really want to ask you to create a separate enum.

Fortunately this will not be a build break in NativeAOT because we already violated the layering (the ObjWriter uses JitInterface constants for stuff).

MichalStrehovsky · 2021-06-10T07:54:42Z

src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs

@@ -53,16 +54,16 @@ public virtual bool IsEffectivelySealed(MethodDesc method)
        /// Note that if <paramref name="implType"/> is a value type, the result of the resolution
        /// might have to be treated as an unboxing thunk by the caller.
        /// </remarks>
-        public MethodDesc ResolveVirtualMethod(MethodDesc declMethod, TypeDesc implType)
+        public MethodDesc ResolveVirtualMethod(MethodDesc declMethod, TypeDesc implType, ref CORINFO_DEVIRTUALIZATION_DETAIL devirtualizationDetail)


Should the new parameter be out instead?

Yes. Turns out not doing this was masking a bug in the devirtualilzation algorithm

MichalStrehovsky · 2021-06-10T07:57:19Z

src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs

+                                {
+                                    // If we devirtualized into a default interface method on a generic type, we should actually return an
+                                    // instantiating stub but this is not happening.
+                                    // Making this work is tracked by https://github.com/dotnet/runtime/issues/9588


This will potentially be a problem because we don't have instantiating stubs in the general type system, same as we don't have unboxing stubs.

We have JitInterface-local unboxing stubs, and I think that will be the solution we'll want to use for instantiating stubs as well, but we will not have access to those from here (we're not in JitInterface).

I don't have a solution, just pointing out if you have thoughts in that area.

Well, its the same problem as devirtualizing to a struct method, which is something that @AndyAyersMS has actually made work fairly recently. I suspect that the comment is somewhat out of date, but I felt it was useful to have a matching comments in both crossgen2 and coreclr vm.

MichalStrehovsky · 2021-06-10T08:03:12Z

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

+                result.tokenContext = jitInterface.contextFromMethod(method);
+                result.token = methodWithToken.Token.Token;
+                if (methodWithToken.Token.TokenType != CorTokenType.mdtMethodDef)
+                    throw new NotSupportedException();


This will tear down the compiler process. Do we want that to happen? Consider RequiresRuntimeJitException (not fatal) or CodeGenerationFailedException (that one can be fatal if we're not in the resilient mode).

MichalStrehovsky · 2021-06-10T08:06:34Z

src/coreclr/tools/Common/TypeSystem/IL/EcmaMethodIL.cs

@@ -11,7 +11,10 @@

 namespace Internal.IL
 {
-    public sealed partial class EcmaMethodIL : MethodIL
+    // Marker interface implemented by EcmaMethodIL and EcmaMethodILScope
+    public interface IEcmaMethodIL { }


Nit: might be faster to just do the two is checks instead of testing for the interface. Both EcmaMethodIL and EcmaMethodILScope are sealed so an is is quick.

…nning different tasks during outerloop

…anonicallly compatible vtable devirtualization, and go back to normal yml for the crossgen2 outerloop

Change dotnet#53567 introduced new functionality to the JIT-EE resolveVirtualMethod() API. This fixes the SuperPMI implementation in the case where the API returns `false`, where some data fields are not initialized. Fixes crossgen2 SuperPMI collections. Fixes dotnet#54310

Change #53567 introduced new functionality to the JIT-EE resolveVirtualMethod() API. This fixes the SuperPMI implementation in the case where the API returns `false`, where some data fields are not initialized. Fixes crossgen2 SuperPMI collections. Fixes #54310

dotnet-issue-labeler bot added the area-crossgen2-coreclr label Jun 2, 2021

MichalStrehovsky and others added 11 commits June 7, 2021 15:22

Re-enable devirtualization in crossgen2 for structs, and increase the…

e631b19

… scenarios in which it is used

Fix most obvious issues

5dbe48a

Handle canonically compatible cases

1695ca7

Add more logic to our devirtualization logic to be ever more certain …

e26075c

…that we won't devirtualize outside of the version bubble

Tweak logic to record tokens for later use. I'm not convinced this is…

09bb941

… ok though, as I think type forwarders can make this R2R unsafe

Rework api surface of devirtualizer to avoid the need to cache token …

8807e82

…info

Don't generate the UpdateContext record for signatures used in instru…

f228d14

…mentation and instantiated tables as the owner type is sufficient for now. - Also, set resolvedTokenDevirtualizedUnboxedMethod correctly

Fix R2RDump decoding in composite images

cf50541

Use correct token

format patch

e923066

davidwrighton force-pushed the fix_devirt_inst_param_issues branch from cced08c to e923066 Compare June 7, 2021 22:29

davidwrighton added 8 commits June 7, 2021 19:38

Decode exact method for decl method

7a14d19

Tweak slot unification to properly ignore name+sig matches on the sam…

2c5ea0a

…e type as a MethodImpl override

Fix issue where we could devirtualize to a method that could not be r…

c3343cd

…eferred to

Validate that the dec method can be represented in a methodsig before…

2a12d01

… it is used in such a way

Update version resiliency rules to be the more precise ones that have…

54e067a

… been determined via analysis

Now that we have a slightly more capable VersionWith predicate for ty…

c9d2212

…pe signature generation, use it in the InstrumentationDataTableNode

- Allow locally defined structs to devirtualize on interface calls ma…

ff3f9b3

…de on non-locally defined interfaces. - decl vs impl check done on exact result, not on canonicalized result

- Ensure all of the devirtualization abort conditions from the runtim…

f4a9153

…e are implemented in crossgen2 - Provide devirtualization details for all possible ways devirt can fail in crossgen2 (This is more detailed than the crossgen output)

davidwrighton marked this pull request as ready for review June 9, 2021 15:50

davidwrighton requested review from AndyAyersMS and trylek June 9, 2021 15:50

AndyAyersMS reviewed Jun 9, 2021

View reviewed changes

trylek approved these changes Jun 9, 2021

View reviewed changes

davidwrighton added 3 commits June 9, 2021 18:05

Code review and formatting

d3601e4

Update jit guid, and superpmi

a9a990e

MichalStrehovsky reviewed Jun 10, 2021

View reviewed changes

davidwrighton added 3 commits June 10, 2021 10:51

Attempt to diagnose outerloop issues by adding instrumentation and ru…

4ab3200

…nning different tasks during outerloop

Fixup issues with virtual resolution of normal vtable methods and a c…

52ece82

…anonicallly compatible vtable devirtualization, and go back to normal yml for the crossgen2 outerloop

Address code review feedback

d71fb6f

davidwrighton merged commit d07f911 into dotnet:main Jun 11, 2021

MichalStrehovsky mentioned this pull request Jun 13, 2021

Merge from dotnet/runtime dotnet/runtimelab#1233

Merged

BruceForstall mentioned this pull request Jun 17, 2021

superpmi crossgen2 libraries collection is broken #54310

Closed

BruceForstall mentioned this pull request Jun 17, 2021

Fix SuperPMI handling of resolveVirtualMethod #54315

Merged

davidwrighton mentioned this pull request Jun 21, 2021

Instantiation argument missing when calling devirtualized valuetype methods #51983

Closed

ghost locked as resolved and limited conversation to collaborators Jul 11, 2021

davidwrighton deleted the fix_devirt_inst_param_issues branch April 13, 2023 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable devirtualization in more scenarios on crossgen2 #53567

Enable devirtualization in more scenarios on crossgen2 #53567

davidwrighton commented Jun 2, 2021 •

edited

Loading

davidwrighton commented Jun 9, 2021

AndyAyersMS left a comment

AndyAyersMS Jun 9, 2021

AndyAyersMS Jun 9, 2021

AndyAyersMS Jun 9, 2021

AndyAyersMS Jun 9, 2021

trylek left a comment

trylek Jun 9, 2021

trylek Jun 9, 2021

trylek Jun 9, 2021

trylek Jun 9, 2021

trylek Jun 9, 2021

trylek Jun 9, 2021

davidwrighton Jun 10, 2021

trylek Jun 9, 2021

davidwrighton Jun 10, 2021

trylek Jun 9, 2021

trylek Jun 9, 2021

trylek Jun 9, 2021

davidwrighton Jun 10, 2021

trylek Jun 9, 2021

MichalStrehovsky Jun 10, 2021

MichalStrehovsky Jun 10, 2021

MichalStrehovsky Jun 10, 2021

davidwrighton Jun 10, 2021

MichalStrehovsky Jun 10, 2021

davidwrighton Jun 10, 2021

MichalStrehovsky Jun 10, 2021

MichalStrehovsky Jun 10, 2021

	comp->impDevirtualizeCall(call, NULL, &method, &methodFlags, &context, nullptr, isLateDevirtualization,
	comp->impDevirtualizeCall(call, nullptr, &method, &methodFlags, &context, nullptr, isLateDevirtualization,

	dvInfo.pResolvedTokenVirtualMethod = NULL;
	dvInfo.pResolvedTokenVirtualMethod = nullptr;

	// 1. At all times the metadata definition of the implmenetation type must version with the application.
	// 1. At all times the metadata definition of the implementation type must version with the application.

	/// Gets the method whose body this <see cref=""/> represents.
	/// Gets the method whose body this <see cref="MethodILScope"/> represents.

		using Internal.TypeSystem;

		using CORINFO_DEVIRTUALIZATION_DETAIL = Internal.JitInterface.CORINFO_DEVIRTUALIZATION_DETAIL;

Enable devirtualization in more scenarios on crossgen2 #53567

Enable devirtualization in more scenarios on crossgen2 #53567

Conversation

davidwrighton commented Jun 2, 2021 • edited Loading

davidwrighton commented Jun 9, 2021

AndyAyersMS 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

trylek 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

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

davidwrighton commented Jun 2, 2021 •

edited

Loading