Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Implement GC.KeepAlive JIT intrinsic#27554

Merged
jkotas merged 2 commits intodotnet:masterfrom
jkotas:keepalive-intrinsics
Nov 1, 2019
Merged

Implement GC.KeepAlive JIT intrinsic#27554
jkotas merged 2 commits intodotnet:masterfrom
jkotas:keepalive-intrinsics

Conversation

@jkotas
Copy link
Member

@jkotas jkotas commented Oct 30, 2019

Fixes #27541

@jkotas
Copy link
Member Author

jkotas commented Oct 30, 2019

cc @dotnet/jit-contrib

@jkotas
Copy link
Member Author

jkotas commented Oct 31, 2019

it would be overkill to mark it with lvAddrExposed.

Agree.

It seems to me that the right semantic for this is that it has global effect (GTF_GLOB_REF) and not mark it as if it's a call.

If I do not mark it as call, fgCheckRemoveStmt will remove it as useless. Is the right approach to special case it in fgCheckRemoveStmt so that it does not get removed?

@AndyAyersMS
Copy link
Member

Is the right approach to special case it in fgCheckRemoveStmt so that it does not get removed?

I'd say yes. Looks like there's precedent there with GT_NOP.

@jkotas jkotas force-pushed the keepalive-intrinsics branch from 5f2a705 to 2166e25 Compare October 31, 2019 22:51
@jkotas
Copy link
Member Author

jkotas commented Oct 31, 2019

jit-diffs

Total bytes of diff: -1021 (-0.03% of base)
    diff is an improvement.
Top file improvements by size (bytes):
       -1021 : System.Private.CoreLib.dasm (-0.03% of base)
1 total files with size differences (1 improved, 0 regressed), 0 unchanged.
Top method improvements by size (bytes):
         -25 (-0.94% of base) : System.Private.CoreLib.dasm - MemberInfoCache`1[__Canon][System.__Canon]:PopulateProperties(Filter,System.RuntimeType,System.Collections.Generic.Dictionary`2[[System.String, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Collections.Generic.List`1[[System.Reflection.RuntimePropertyInfo, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]],System.Boolean[],byref):this
         -24 (-9.45% of base) : System.Private.CoreLib.dasm - ILStubClass:IL_STUB_PInvoke(EnumLocalesProcEx,int,long,long):bool
         -24 (-8.76% of base) : System.Private.CoreLib.dasm - ILStubClass:IL_STUB_PInvoke(EnumTimeFormatsProcEx,System.String,int,long):bool
         -24 (-7.74% of base) : System.Private.CoreLib.dasm - ILStubClass:IL_STUB_PInvoke(EnumCalendarInfoProcExEx,System.String,int,System.String,int,long):bool
         -23 (-3.65% of base) : System.Private.CoreLib.dasm - System.Delegate:GetMethodImpl():System.Reflection.MethodInfo:this
Top method improvements by size (percentage):
         -15 (-75.00% of base) : System.Private.CoreLib.dasm - System.StubHelpers.KeepAliveCleanupWorkListElement:DestroyCore():this
         -16 (-39.02% of base) : System.Private.CoreLib.dasm - System.Reflection.RuntimeConstructorInfo:get_Name():System.String:this
         -14 (-35.90% of base) : System.Private.CoreLib.dasm - System.GC:GetGeneration(System.WeakReference):int
         -14 (-34.15% of base) : System.Private.CoreLib.dasm - System.Reflection.RuntimeMethodInfo:get_IsGenericMethod():bool:this
         -14 (-34.15% of base) : System.Private.CoreLib.dasm - System.Reflection.RuntimeMethodInfo:get_IsGenericMethodDefinition():bool:this
75 total methods with size differences (75 improved, 0 regressed), 25572 unchanged.

@jkotas
Copy link
Member Author

jkotas commented Oct 31, 2019

You also need to change the LSRA code to call BuildOperandUses instead of BuildUse

Thank you for your help! This works great on x86/x64.

On arm, I am getting "Assertion failed '!"Found mismatched live reg var(s) after block". Are the optional regs supposed to work on arm? They seem to be used very rarely on arm.

@jkotas jkotas marked this pull request as ready for review October 31, 2019 23:58
@jkotas
Copy link
Member Author

jkotas commented Oct 31, 2019

This is ready to go except the arm specific issue with optional regs.

@CarolEidt
Copy link

On arm, I am getting "Assertion failed '!"Found mismatched live reg var(s) after block". Are the optional regs supposed to work on arm? They seem to be used very rarely on arm.

They are indeed used very rarely on arm, since there aren't any instructions other than loads that can load a lclVar. So the code that would normally kill a last-use may not kick in if it doesn't get a register.

@CarolEidt
Copy link

Indeed, the code that handles a "contained" local read is #ifdefd out on arm/arm64.
I think that for the armarch version instead of genConsumeRegs you could use:

if (tree->isContained())
{
    // For this case we simply need to update the lifetime of the local.
    genUpdateLife(tree);
}
else
{
    genConumeReg(tree);
}

You could add this logic to the armarch version of genConsumeRegs but since this would be the only case to use it, it doesn't seem fruitful.

@jkotas
Copy link
Member Author

jkotas commented Nov 1, 2019

@CarolEidt This did the trick. Thank you!

@jkotas
Copy link
Member Author

jkotas commented Nov 1, 2019

@dotnet/jit-contrib Could you please take a final look and approve the change?

Copy link
Member

@erozenfeld erozenfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jkotas jkotas merged commit c3dc543 into dotnet:master Nov 1, 2019
@jkotas jkotas deleted the keepalive-intrinsics branch November 3, 2019 14:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GC.KeepAlive JIT intrinsic

8 participants