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

Fix silent bad codegen VSD possible tailcall converted to normal call #49256

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

BruceForstall
Copy link
Member

@BruceForstall BruceForstall commented Mar 6, 2021

The problem was when a VSD interface call returning a multi-byte struct
in registers was initially considered to be a tailcall, but the tailcall
was abandoned in morph due to not enough stack space to store outgoing
arguments, in which case we create a new call return local to store the
return struct, and re-morph the call. In doing so, we forget that we had
already added VSD non-standard args, and re-add them, leaving the originally
added arg as a "normal" arg that shouldn't be there.

So, in summary, for a call A->B, to see this failure, we need:

  1. The call is considered a potential tailcall (by the importer)
  2. The call requires non-standard arguments that add call argument IR in
    fgInitArgInfo() (e.g., VSD call -- in this case, a generic interface call)
  3. We reject the tailcall in fgMorphPotentialTailCall() (e.g., not enough
    incoming arg stack space in A to store B's outgoing args), in this case
    because the first arg is a large struct. We can't reject it earlier,
    due to things like address exposed locals -- we must get far enough
    through the checks to have called fgInitArgInfo() to add the extra
    non-standard arg.
  4. B returns a struct in multiple registers (e.g., a 16-byte struct in
    Linux x64 ABI)

The fix is to remove the previously-added non-standard VSD argument IR when
we are preparing to re-morph a call.

There was one other location that was already doing this. I'm a little
worried that there are other places where the non-standard added IR is
not being considered when we clear out the arg info before remorphing.
It seems like there is some risk here. Probably, we should consider a
better way of handling the non-standard arg IR given the need in some
cases to re-morph calls.

I commented out a few cases of:

assert(!fgCanFastTailCall(call, nullptr));

because this function can call fgInitArgInfo which can alter the IR.
That seems dangerous in an assert, which should have any side-effects
like modifying the IR.

Fixes #49078

No SPMI asm diffs.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 6, 2021
@BruceForstall BruceForstall changed the title Add unit test for #49078 Fix silent bad codegen VSD possible tailcall converted to normal call Mar 8, 2021
@BruceForstall
Copy link
Member Author

/azp run runtime-coreclr outerloop

@dotnet dotnet deleted a comment from azure-pipelines bot Mar 8, 2021
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BruceForstall BruceForstall requested a review from sandreenko March 8, 2021 00:42
@BruceForstall
Copy link
Member Author

@dotnet/jit-contrib PTAL

// to store B's outgoing args), in this case because the first arg is a large struct. We can't reject
// it earlier, due to things like address exposed locals -- we must get far enough through the checks
// to have called fgInitArgInfo() to add the extra non-standard arg.
// 4. B returns a struct in multiple registers (e.g., a 16-byte struct in Linux x64 ABI)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the 4. required for an additional call to morph the tree?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Only in that case does the code add a new return value and re-morph the tree.

The problem was when a VSD interface call returning a multi-byte struct
in registers was initially considered to be a tailcall, but the tailcall
was abandoned in morph due to not enough stack space to store outgoing
arguments, in which case we create a new call return local to store the
return struct, and re-morph the call. In doing so, we forget that we had
already added VSD non-standard args, and re-add them, leaving the originally
added arg as a "normal" arg that shouldn't be there.

So, in summary, for a call A->B, to see this failure, we need:
1. The call is considered a potential tailcall (by the importer)
2. The call requires non-standard arguments that add call argument IR in
   fgInitArgInfo() (e.g., VSD call -- in this case, a generic interface call)
3. We reject the tailcall in fgMorphPotentialTailCall() (e.g., not enough
   incoming arg stack space in A to store B's outgoing args), in this case
   because the first arg is a large struct. We can't reject it earlier,
   due to things like address exposed locals -- we must get far enough
   through the checks to have called fgInitArgInfo() to add the extra
   non-standard arg.
4. B returns a struct in multiple registers (e.g., a 16-byte struct in
   Linux x64 ABI)

The fix is to remove the previously-added non-standard VSD argument IR when
we are preparing to re-morph a call.

There was one other location that was already doing this. I'm a little
worried that there are other places where the non-standard added IR is
not being considered when we clear out the arg info before remorphing.
It seems like there is some risk here. Probably, we should consider a
better way of handling the non-standard arg IR given the need in some
cases to re-morph calls.

I commented out a few cases of:
```
assert(!fgCanFastTailCall(call, nullptr));
```
because this function can call `fgInitArgInfo` which can alter the IR.
That seems dangerous in an assert, which should have any side-effects
like modifying the IR.

Fixes dotnet#49078

No SPMI asm diffs.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible OSX/Linux JIT Inlining Optimization Bug .Net Core 5.0.200
5 participants