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

JIT: enable implicit tail calls from inlined code #9405

Merged
merged 2 commits into from Feb 10, 2017

Conversation

AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Feb 8, 2017

Inlines of calls from implicit tail call sites should allow recognition
of inlinee implicit tail call sites.

The jit recognizes implicit tail call sites during importation,
but the inlinee compiler instance did not have compTailCallOpt set
and so never recognized these instances. Fix this and update the logic
to detect the transitively implicit tail calls.

Now that these sites are recognized, morph needs a fix to tunnel through
repeated casts for tail calls, since each level of inlining might add a
cast. All these casts should be identical.

Note under R2R tail calls are not yet recognized (see ZapInfo::canTailCall).

Closes #9349.

@AndyAyersMS
Copy link
Member Author

@AndyAyersMS AndyAyersMS commented Feb 8, 2017

@JosephTremoulet PTAL
cc @dotnet/jit-contrib

Jit-diff data (only diffs in corelib because R2R currently inhibits tail calls):

Total bytes of diff: 798 (0.01 % of base)
    diff is a regression.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
         798 : System.Private.CoreLib.dasm (0.02 % of base)
1 total files with size differences.
Top method regessions by size (bytes):
          47 : System.Private.CoreLib.dasm - System.Diagnostics.Tracing.ActivityTracker:OnStart(ref,ref,int,byref,byref,int):this
          40 : System.Private.CoreLib.dasm - System.Diagnostics.Tracing.ActivityTracker:OnStop(ref,ref,int,byref):this
          27 : System.Private.CoreLib.dasm - System.StubHelpers.NullableMarshaler:ConvertToNative(byref):long (9 methods)
          20 : System.Private.CoreLib.dasm - System.Security.Permissions.FileIOPermission:AddPathList(int,int,ref,bool,bool,bool):this
          12 : System.Private.CoreLib.dasm - System.Reflection.Emit.TypeBuilder:SetInterfaces(ref):this
Top method improvements by size (bytes):
         -34 : System.Private.CoreLib.dasm - System.Reflection.Emit.SignatureHelper:AddOneArgTypeHelperWorker(ref,bool):this
         -10 : System.Private.CoreLib.dasm - Decoder:.ctor(ref):this (2 methods)
          -5 : System.Private.CoreLib.dasm - UTF32Decoder:.ctor(ref):this
          -5 : System.Private.CoreLib.dasm - System.Nullable`1[Guid][System.Guid]:ToString():ref:this
          -5 : System.Private.CoreLib.dasm - System.Nullable`1[TimeSpan][System.TimeSpan]:ToString():ref:this
384 total methods with size differences.

@AndyAyersMS
Copy link
Member Author

@AndyAyersMS AndyAyersMS commented Feb 8, 2017

Tail call opts are not enabled in debug builds, so even w/o looking in the logs, the ARM64 failures are almost certainly unrelated. Looking at the logs shows some kind of scripting issue.

18:56:27 d:\j\workspace\arm64_cross_c---f41ee988>python tests\scripts\arm64_post_build.py -repo_root d:\j\workspace\arm64_cross_c---f41ee988 -arch arm64 -build_type checked -scenario default -key_location C:\tools\key.txt 
18:56:27 Traceback (most recent call last):
18:56:27   File "tests\scripts\arm64_post_build.py", line 311, in <module>
18:56:27     main(args)
18:56:27   File "tests\scripts\arm64_post_build.py", line 254, in main
18:56:27     repo_root, arch, build_type, scenario, key_location, force_update = validate_args(args)
18:56:27   File "tests\scripts\arm64_post_build.py", line 228, in validate_args
18:56:27     validate_arg(key_location, lambda item: os.path.isfile(item))
18:56:27   File "tests\scripts\arm64_post_build.py", line 218, in validate_arg
18:56:27     raise Exception("Argument: %s is not valid." % (arg))
18:56:27 Exception: Argument: C:\tools\key.txt is not valid.

Copy link

@JosephTremoulet JosephTremoulet left a comment

LGTM

CLANG_FORMAT_COMMENT_ANCHOR;

#if FEATURE_TAILCALL_OPT
opts.compTailCallLoopOpt = true;

Choose a reason for hiding this comment

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

Would it be possible/interesting (at some point in the future) to recognize tail calls back to an inlinee and make them loops?

Copy link
Member Author

@AndyAyersMS AndyAyersMS Feb 8, 2017

Choose a reason for hiding this comment

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

Yes, it would be possible. We already track the ancestral inline context for each call site, so we can tell when a call site recursively invokes an inlinee. What we'd need in addition is the following:

  • track the logical entry point for the inlinee in the inline context, so we know where to target the loop branch.
  • disable direct sub for inlinees in tail position, since the arguments are now potentially starg.

With that I think we could generalize the loop tail call opt to handle inlined cases, and I think a tail call loop opt is potentially much more impactful then a tail call alone.

Copy link
Member Author

@AndyAyersMS AndyAyersMS Feb 8, 2017

Choose a reason for hiding this comment

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

One more thing: we currently won't tail call if the root method is noinline, but that block doesn't make sense for these inlinee loop tail call opts, so we should track calls in tail position in addition to calls eligible to be made into tail calls.

@sivarv
Copy link
Member

@sivarv sivarv commented Feb 8, 2017

Looks good.
Trying ngen diffs on desktop will provide another measure of CQ impact.
On desktop, tail call differences could lead to app compat issues (we had an instance of GetCallingAssembly() failure during 4.6 sign-off time). We can be conservative and enable this only for CoreCLR or should remember to disable this if there are any app compat failures on desktop due to this.

@AndyAyersMS
Copy link
Member Author

@AndyAyersMS AndyAyersMS commented Feb 8, 2017

@JosephTremoulet I vaguely recall you had a way of getting fragile ngen codegen in jit-diffs. If not, we should potentially add that as an option. Or get SPMI online and ensure it has a proper mix of jit/ngen/r2r instances.

@sivarv good point about desktop. Do you recall what the fix was for the app compat issue?

@JosephTremoulet
Copy link

@JosephTremoulet JosephTremoulet commented Feb 8, 2017

@JosephTremoulet I vaguely recall you had a way of getting fragile ngen codegen in jit-diffs. If not, we should potentially add that as an option

No, I've just used jit-produced asm when I wanted to be extra sure I was looking at diffs matching benchmark execution. You may be remembering that I opened dotnet/jitutils#75, but I haven't implemented it. It should be trivial to implement if crossgen has a --fragile flag, but if it has one it seems to be hidden...

Or get SPMI online and ensure it has a proper mix of jit/ngen/r2r instances.

That would be great!

Inlines of calls from implicit tail call sites should allow recognition
of inlinee implicit tail call sites.

The jit recognizes implicit tail call sites during importation,
but the inlinee compiler instance did not have compTailCallOpt set
and so never recognized these instances. Fix this and update the logic
to detect the transitively implicit tail calls.

Now that these sites are recognized, morph needs a fix to tunnel through
repeated casts for tail calls, since each level of inlining might add a
cast. All these casts should be identical.

Note under R2R tail calls are not yet recognized (see ZapInfo::canTailCall).

Closes #9349.
@AndyAyersMS
Copy link
Member Author

@AndyAyersMS AndyAyersMS commented Feb 8, 2017

crossgen has a /FragileNonVersionable option for this, though it doesn't show up in the help.

I would patch jit-dasm/jit-diff but I'm stuck on an older version....

@sivarv
Copy link
Member

@sivarv sivarv commented Feb 8, 2017

@AndyAyersMS - desktop appcompat failure was due to a particular class of implicit tail calls (the ones with shared return). See FEATURE_TAILCALL_OPT_SHARED_RETURN in jit.h, it is defined as 1 for CoreCLR and 0 on desktop.

@AndyAyersMS
Copy link
Member Author

@AndyAyersMS AndyAyersMS commented Feb 8, 2017

Tried running desktop SPMI. Unfortunately it fails on exactly the methods we'd like to examine, since there are new calls to canTailCall that aren't resolvable by the cached data. So will try ngen or PMI diffs instead.

@AndyAyersMS
Copy link
Member Author

@AndyAyersMS AndyAyersMS commented Feb 8, 2017

Desktop also running into asserts because for desktop FEATURE_TAILCALL_OPT_SHARED_RETURN is disabled, and it's not guaranteed that inline implicit tail call sites are in a BBJ_RETURN block (they may be in a predecessor block that falls through or jumps to the return). It seems prudent for now to only enable recognition of inline implicit tail calls under this feature.

Which means, they won't be recognized on desktop....

@AndyAyersMS AndyAyersMS force-pushed the InlineImplicitTailCalls branch from 9af0cf8 to 3387edc Compare Feb 9, 2017
@AndyAyersMS
Copy link
Member Author

@AndyAyersMS AndyAyersMS commented Feb 9, 2017

Updated (& rebased) to only kick in if we support shared returns. Verified no diff on desktop.

@sivarv
Copy link
Member

@sivarv sivarv commented Feb 10, 2017

Looks good.

@AndyAyersMS AndyAyersMS merged commit 1a8cb67 into dotnet:master Feb 10, 2017
14 checks passed
sdmaclea pushed a commit to sdmaclea/coreclr that referenced this issue Feb 15, 2017
Inlines of calls from implicit tail call sites should allow recognition
of inlinee implicit tail call sites.

The jit recognizes implicit tail call sites during importation,
but the inlinee compiler instance did not have compTailCallOpt set
and so never recognized these instances. Fix this and update the logic
to detect the transitively implicit tail calls.

Now that these sites are recognized, morph needs a fix to tunnel through
repeated casts for tail calls, since each level of inlining might add a
cast. All these casts should be identical.

Note under R2R tail calls are not yet recognized (see ZapInfo::canTailCall).

Enable only under FEATURE_TAILCALL_OPT_SHARED_RETURN since the
inline tail call sites are not likely to be in BBJ_RETURN blocks.

Closes #9349.
jorive pushed a commit to guhuro/coreclr that referenced this issue May 4, 2017
Inlines of calls from implicit tail call sites should allow recognition
of inlinee implicit tail call sites.

The jit recognizes implicit tail call sites during importation,
but the inlinee compiler instance did not have compTailCallOpt set
and so never recognized these instances. Fix this and update the logic
to detect the transitively implicit tail calls.

Now that these sites are recognized, morph needs a fix to tunnel through
repeated casts for tail calls, since each level of inlining might add a
cast. All these casts should be identical.

Note under R2R tail calls are not yet recognized (see ZapInfo::canTailCall).

Enable only under FEATURE_TAILCALL_OPT_SHARED_RETURN since the
inline tail call sites are not likely to be in BBJ_RETURN blocks.

Closes #9349.
@karelz karelz added this to the 2.0.0 milestone Aug 28, 2017
@karelz karelz added this to the 2.0.0 milestone Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this issue Feb 18, 2022
Inlines of calls from implicit tail call sites should allow recognition
of inlinee implicit tail call sites.

The jit recognizes implicit tail call sites during importation,
but the inlinee compiler instance did not have compTailCallOpt set
and so never recognized these instances. Fix this and update the logic
to detect the transitively implicit tail calls.

Now that these sites are recognized, morph needs a fix to tunnel through
repeated casts for tail calls, since each level of inlining might add a
cast. All these casts should be identical.

Note under R2R tail calls are not yet recognized (see ZapInfo::canTailCall).

Enable only under FEATURE_TAILCALL_OPT_SHARED_RETURN since the
inline tail call sites are not likely to be in BBJ_RETURN blocks.

Closes dotnet/coreclr#9349.


Commit migrated from dotnet/coreclr@1a8cb67
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants