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

C#: Move implicit entry definitions inside method bodies in SSA construction #16875

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Jun 28, 2024

Methods with multiple implementations such as

// File1.cs
class MultiImpl
{
    public int M(int x) => x;
}

// File2.cs
class MultiImpl
{
    public int M(int x) => x;
}

give rise to a single CFG, where flow splits immediately following the special entry node:

flowchart TD
1["enter M"]
2["exit M"]
3["exit M (normal)"]
4["access to parameter x"]
5["access to parameter x"]

1 --> 4
3 --> 2
4 --> 3
5 --> 3
1 --> 5
Loading

Before this PR, implicit entry SSA definitions (such as for parameters) were placed at the enter M node, meaning that all implementation bodies would share those definitions. However, in #16817 we would like to be able to differentiate between the method bodies, and this PR consequently moves the implicit entry definitions into the basic blocks that start each method body (i.e., the access to parameter x nodes above).

Note that single-bodied methods (which is the far most common case) will still have the implicit entry SSA definitions at the entry nodes, since the basic block for the method body entry is the same as the basic block for the special entry node.

@github-actions github-actions bot added the C# label Jun 28, 2024

bindingset[l1, l2]
pragma[inline_late]
private predicate inSameFile0(Location l1, Location l2) { l1.getFile() = l2.getFile() }

Check warning

Code scanning / CodeQL

Candidate predicate not marked as `nomagic` Warning

Candidate predicate to
inSameFile
is not marked as nomagic.
@hvitved hvitved force-pushed the csharp/ssa-param-def branch 3 times, most recently from f765570 to f9324d1 Compare July 1, 2024 08:06
@hvitved hvitved changed the title C#: Move implicit parameter definitions inside method bodies in SSA construction C#: Move implicit entry definitions inside method bodies in SSA construction Jul 1, 2024
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Jul 1, 2024
@hvitved hvitved marked this pull request as ready for review July 1, 2024 09:22
@hvitved hvitved requested a review from a team as a code owner July 1, 2024 09:22
@@ -50,6 +50,27 @@ class Location extends @location {

/** Gets the 1-based column number (inclusive) where this location ends. */
final int getEndColumn() { this.hasLocationInfo(_, _, _, _, result) }

/** Holds if this location starts strictly before the specified location. */
bindingset[this, other]
Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, so it is possible to declare bindingsets for member predicates?
Or is the bindingset just declared because you want to use the inline_late pragma?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's needed for inline_late.

Comment on lines +56 to +57
assignableDefinitionLocalScopeVariable(result, v, c) and
localScopeSourceVariable(this, v, c, _)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you need to force the join order for performance reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

predicate implicitEntryDefinition(ControlFlow::ControlFlow::BasicBlock bb, Ssa::SourceVariable v) {
exists(ControlFlow::ControlFlow::BasicBlocks::EntryBlock entry, Callable c |
c = entry.getCallable() and
// In case `c` has multiple bodies, we want each body to gets its own implicit
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: get its own

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll fix follow-up on another PR to avoid having to wait for CI.

}

override Location getLocation() {
not NearestLocation<NearestLocationInput>::nearestLocation(this, _, _) and
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't this give us the exact location whe the is only a single implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the

not def.getBasicBlock() instanceof ControlFlow::BasicBlocks::EntryBlock

constraint in NearestLocationInput::relevantLocations/3 restricts the logic to multi-bodied methods.

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

Looks plausible to me!

@hvitved hvitved merged commit d675304 into github:main Jul 4, 2024
22 checks passed
@hvitved hvitved deleted the csharp/ssa-param-def branch July 4, 2024 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants