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

Update lsra-detail.md #45235

Merged
merged 6 commits into from
Dec 1, 2020
Merged

Update lsra-detail.md #45235

merged 6 commits into from
Dec 1, 2020

Conversation

CarolEidt
Copy link
Contributor

No description provided.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 25, 2020
@CarolEidt
Copy link
Contributor Author

@dotnet/jit-contrib PTAL

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM with some small suggestions.

@@ -308,6 +309,7 @@ After LSRA, the graph has the following properties:
must issue the appropriate move.

- However, if such a node is constrained to a set of registers,
as in the case of x86 instructions which require a byte-addressible register,
Copy link
Member

Choose a reason for hiding this comment

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

nit: addressable

@@ -321,7 +323,12 @@ After LSRA, the graph has the following properties:

- Note that a write-thru variable def is always written to the stack, and the `GTF_SPILLED`
Copy link
Member

Choose a reason for hiding this comment

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

while you are here: "and it is marked as GTF_SPILLED"

@@ -321,7 +323,12 @@ After LSRA, the graph has the following properties:

- Note that a write-thru variable def is always written to the stack, and the `GTF_SPILLED`
flag (not otherwise used for pure defs) to indicate that it also remains live
in the assigned register.
in the assigned register. This is somewhat counter-intuitive, but
Copy link
Member

Choose a reason for hiding this comment

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

May be we should move this after we describe what a node with GTF_SPILLED means and further re-phrase as :

"If a node has both GTF_SPILL and GTF_SPILLED, the tree node is spilled after it is evaluated (GTF_SPILL) and reloaded prior to the use (GTF_SPILLED). After the node is spilled and before the reload, its value is not live in the register. However, for EH-write-thru variables, there is an exception. The variable def is always written to the stack (GTF_SPILL), should always be reloaded (GTF_SPILLED) and always remain live in the register as well."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, though the description should be reordered to make it clear that the reloading comes first in the general case.

would require us to spill the lclVar and reload it for the next use.
Instead, we retain the assignment of the register to the lclVar, and mark that `RegRecord` as
`isBusyUntilNextKill` so that it isn't reused if the lclVar goes dead before the call.
(Otherwise, if the either the lclVar is in a different register, or if its next use is after
Copy link
Member

Choose a reason for hiding this comment

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

nit: Otherwise, if the lclVar is...

the register assignments across edges.

- The critical edges are handled first. These are the most problematic, as there
is no single location at which the moves can be added. We handle the critical
Copy link
Member

Choose a reason for hiding this comment

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

This sentence doesn't sound right. Did you mean "We handle all the critical edges that originates from the source block"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, all the critical edges are handled first. I'll work on a rephrasing of this.

@kunalspathak
Copy link
Member

Changes looks good. Thanks!

@CarolEidt CarolEidt merged commit 301056d into dotnet:master Dec 1, 2020
@CarolEidt CarolEidt deleted the UpdateLsraDoc branch December 1, 2020 14:00
@ghost ghost locked as resolved and limited conversation to collaborators Dec 31, 2020
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.

3 participants