Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RyuJIT: IsValueType and IsAssignableFrom optimizations aren't inlining friendly. #40381

Closed
EgorBo opened this issue Aug 5, 2020 · 1 comment · Fixed by #71778
Closed

RyuJIT: IsValueType and IsAssignableFrom optimizations aren't inlining friendly. #40381

EgorBo opened this issue Aug 5, 2020 · 1 comment · Fixed by #71778
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI tenet-performance Performance related issue
Milestone

Comments

@EgorBo
Copy link
Member

EgorBo commented Aug 5, 2020

#1157 introduced a jit optimization to optimize typeof(X).IsValueType to true/false. However, it seems this optimization doesn't work with inlining (same for IsAssignableFrom), e.g.:

bool Test() => Foo(typeof(int));

bool Foo(Type t1) => t1.IsValueType;

Codegen for Test (with Foo inlined):

G_M31255_IG01:
       4883EC28             sub      rsp, 40
G_M31255_IG02:
       48B9A09BC9ECF87F0000 mov      rcx, 0x7FF8ECC99BA0
       E82D238D5F           call     CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
       488BC8               mov      rcx, rax
       48B8D8A8BEECF87F0000 mov      rax, 0x7FF8ECBEA8D8
       488B00               mov      rax, qword ptr [rax]
       3909                 cmp      dword ptr [rcx], ecx
G_M31255_IG03:
       4883C428             add      rsp, 40
       48FFE0               rex.jmp  rax

Expected codegen:

       mov      eax, 1   ;; return true
       ret  

the problem here is the fact that when we inline Foo we save typeof(int) into a temp variable (GT_LCL_VAR) and then importer is not able to optimize get_IsValueType because it expects input to be typeof(X) (CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE) rather than an unknown local, here is the IR after inlining:

# var tmp = typeof(x);
*  ASG       ref   
+--*  LCL_VAR   ref    V02 tmp1         
\--*  CALL help ref    HELPER.CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
   \--*  CNS_INT(h) long   0x7ff8ecc99ba0 class


# return tmp.IsValueType;
*  CALL void   System.RuntimeType.IsValueTypeImpl
\--*  LCL_VAR   ref    V02 tmp1  

if we could, instead of saving typeof(x) into a temp propogate it by value during inlining I believe we'd get:

*  CALL void   System.RuntimeType.IsValueTypeImpl
\--*  CALL help ref    HELPER.CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
   \--*  CNS_INT(h) long   0x7ff8ecc99ba0 class

Other options: early CSE ? or when I import get_IsValueType and see LCL_VAR arg with lvSingleDef flag - can I quickly get its assignment node?

PS: the same issue prevents the new API Type.IsAssignableTo to be typeof(X) friendly since it uses IsAssignableFrom under the hood.

/cc @dotnet/jit-contrib

category:cq
theme:inlining
skill-level:expert
cost:large

@EgorBo EgorBo added the tenet-performance Performance related issue label Aug 5, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Aug 5, 2020
@AndyAyersMS
Copy link
Member

propogate it by value during inlining

This happens in impInlineFetchArg, which tries to decide if an arg reference in an inlinee can simply refer directly to the argument value passed by the caller. Most simple tree nodes are already handled this way.

For more complex nodes the typical gotchas are side effect ordering (and in particular exception ordering), and the possibility of widespread code duplication.

I have a prototype branch InlineForwardSubSingleUseWithException that allows more complex trees to be substituted (to address #4207). Trees with exception effects but no global effects can be handled by evaluating them both at the call site and at the use point, with the hope that both will get optimized. My notes say "One issue is that we'll dup arbitrarily large trees, might need to focus more on profitability." Note early on in the jit we don't have a good notion of how much computation a tree represents.

@AndyAyersMS AndyAyersMS removed the untriaged New issue has not been triaged by the area owner label Aug 5, 2020
@AndyAyersMS AndyAyersMS added this to the Future milestone Aug 5, 2020
@BruceForstall BruceForstall added the JitUntriaged CLR JIT issues needing additional triage label Oct 28, 2020
@BruceForstall BruceForstall removed the JitUntriaged CLR JIT issues needing additional triage label Nov 13, 2020
@EgorBo EgorBo modified the milestones: Future, 7.0.0 Jul 8, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 8, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 10, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants