Handle LCL_FLD_ADDR in TreeLifeUpdater

[SingleAccretion]

Credit to @BruceForstall for investigating the issue.

Fixes#79518.

No code diffs, but some GC info diffs that look like fixed GC holes. Some TP losses on 32 bit, payed for with #79194.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Credit to @BruceForstall for investigating the issue.

Fixes #79518.

No code diffs, but some GC info diffs that look like fixed GC holes.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[BruceForstall]

I understand STOREIND{ref}(LCL_VAR_ADDR{byref}[V21], ref value): V21 goes GC live at this location.

What about:

• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 2], ref value) => illegal, ref needs to be aligned
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value) (on 64-bit) => the 2nd field of a non-promoted struct type local V21 is a ref? Then liveness will mark the "entire" V21 live after this? But non-promoted structs are always GC untracked. So it seems like this should be illegal, too.

[SingleAccretion]

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?

It could also be a ref-typed local itself, or even byref-typed (since refs are a subset of byrefs). Or it could be a larger struct such as in your 3rd example.

Ultimately, we could lower away LCL_FLD_ADDR [0] to LCL_VAR_ADDR, but it does not seem like a good idea to me to make that mandatory. For example, it would only add code to this change, we would still need to handle LCL_FLD_ADDR in the tree life updater.

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value), Then liveness will mark the "entire" V21 live after this?

Most commonly, yes, except for dead stores.

As you note, the GC tracking would not be affected either way since the underlying local must be a struct hence GC-untracked else it is UB.

I am not sure why should it be illegal? One can write code like that today, and it will work as expected. Edit: I suppose you meant that it would never result in GC info updates. That it true.

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

[BruceForstall]

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking. for STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them. It seems now that more node types need to handle struct fields of GC type updates to the codegen-tracked GC info.

[SingleAccretion]

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking

The only change of significance was that we can now see LCL_FLD_ADDR [0] at a zero offset where previously we would have seen LCL_VAR_ADDR. Ultimately, this is a result of a number of changes, but since the two nodes have exactly identical semantics, it ought not to matter modulo bugs such as this one.

STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them.

That is not quite the case, STORE_LCL_FLD has been handling GC types for a very long time, it just doesn't matter a lot for it because we would convert all zero-offset, off-of-GC-local LCL_FLDs into LCL_VARs in most circumstances (notably, not always, due to ref <-> byref reinterpretation), and then the rest would go into GC-untracked struct locals. OBJs could "obscure" the underlying TYP_REF local a little better (due to some copy block morphing inefficiencies) hence we see (and saw) this pattern with them more.

[SingleAccretion]

@dotnet/jit-contrib

[BruceForstall]

I'll go ahead and approve this; it fixes an existing regression.

I'm still uncomfortable with how LCL_FLD and GC tracking are currently interacting. It seems like LCL_FLD should never be of a GC type. Unless the JIT tracks individual struct local fields and reports the corresponding GC-typed fields as slots to the GC.

[SingleAccretion]

Credit to @BruceForstall for investigating the issue.

Fixes#79518.

No code diffs, but some GC info diffs that look like fixed GC holes. Some TP losses on 32 bit, payed for with #79194.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Credit to @BruceForstall for investigating the issue.

Fixes #79518.

No code diffs, but some GC info diffs that look like fixed GC holes.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[BruceForstall]

I understand STOREIND{ref}(LCL_VAR_ADDR{byref}[V21], ref value): V21 goes GC live at this location.

What about:

• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 2], ref value) => illegal, ref needs to be aligned
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value) (on 64-bit) => the 2nd field of a non-promoted struct type local V21 is a ref? Then liveness will mark the "entire" V21 live after this? But non-promoted structs are always GC untracked. So it seems like this should be illegal, too.

[SingleAccretion]

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?

It could also be a ref-typed local itself, or even byref-typed (since refs are a subset of byrefs). Or it could be a larger struct such as in your 3rd example.

Ultimately, we could lower away LCL_FLD_ADDR [0] to LCL_VAR_ADDR, but it does not seem like a good idea to me to make that mandatory. For example, it would only add code to this change, we would still need to handle LCL_FLD_ADDR in the tree life updater.

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value), Then liveness will mark the "entire" V21 live after this?

Most commonly, yes, except for dead stores.

As you note, the GC tracking would not be affected either way since the underlying local must be a struct hence GC-untracked else it is UB.

I am not sure why should it be illegal? One can write code like that today, and it will work as expected. Edit: I suppose you meant that it would never result in GC info updates. That it true.

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

[BruceForstall]

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking. for STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them. It seems now that more node types need to handle struct fields of GC type updates to the codegen-tracked GC info.

[SingleAccretion]

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking

The only change of significance was that we can now see LCL_FLD_ADDR [0] at a zero offset where previously we would have seen LCL_VAR_ADDR. Ultimately, this is a result of a number of changes, but since the two nodes have exactly identical semantics, it ought not to matter modulo bugs such as this one.

STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them.

That is not quite the case, STORE_LCL_FLD has been handling GC types for a very long time, it just doesn't matter a lot for it because we would convert all zero-offset, off-of-GC-local LCL_FLDs into LCL_VARs in most circumstances (notably, not always, due to ref <-> byref reinterpretation), and then the rest would go into GC-untracked struct locals. OBJs could "obscure" the underlying TYP_REF local a little better (due to some copy block morphing inefficiencies) hence we see (and saw) this pattern with them more.

[SingleAccretion]

@dotnet/jit-contrib

[BruceForstall]

I'll go ahead and approve this; it fixes an existing regression.

I'm still uncomfortable with how LCL_FLD and GC tracking are currently interacting. It seems like LCL_FLD should never be of a GC type. Unless the JIT tracks individual struct local fields and reports the corresponding GC-typed fields as slots to the GC.

[SingleAccretion]

Credit to @BruceForstall for investigating the issue.

Fixes#79518.

No code diffs, but some GC info diffs that look like fixed GC holes. Some TP losses on 32 bit, payed for with #79194.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Credit to @BruceForstall for investigating the issue.

Fixes #79518.

No code diffs, but some GC info diffs that look like fixed GC holes.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[BruceForstall]

I understand STOREIND{ref}(LCL_VAR_ADDR{byref}[V21], ref value): V21 goes GC live at this location.

What about:

• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 2], ref value) => illegal, ref needs to be aligned
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value) (on 64-bit) => the 2nd field of a non-promoted struct type local V21 is a ref? Then liveness will mark the "entire" V21 live after this? But non-promoted structs are always GC untracked. So it seems like this should be illegal, too.

[SingleAccretion]

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?

It could also be a ref-typed local itself, or even byref-typed (since refs are a subset of byrefs). Or it could be a larger struct such as in your 3rd example.

Ultimately, we could lower away LCL_FLD_ADDR [0] to LCL_VAR_ADDR, but it does not seem like a good idea to me to make that mandatory. For example, it would only add code to this change, we would still need to handle LCL_FLD_ADDR in the tree life updater.

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value), Then liveness will mark the "entire" V21 live after this?

Most commonly, yes, except for dead stores.

As you note, the GC tracking would not be affected either way since the underlying local must be a struct hence GC-untracked else it is UB.

I am not sure why should it be illegal? One can write code like that today, and it will work as expected. Edit: I suppose you meant that it would never result in GC info updates. That it true.

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

[BruceForstall]

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking. for STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them. It seems now that more node types need to handle struct fields of GC type updates to the codegen-tracked GC info.

[SingleAccretion]

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking

The only change of significance was that we can now see LCL_FLD_ADDR [0] at a zero offset where previously we would have seen LCL_VAR_ADDR. Ultimately, this is a result of a number of changes, but since the two nodes have exactly identical semantics, it ought not to matter modulo bugs such as this one.

STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them.

That is not quite the case, STORE_LCL_FLD has been handling GC types for a very long time, it just doesn't matter a lot for it because we would convert all zero-offset, off-of-GC-local LCL_FLDs into LCL_VARs in most circumstances (notably, not always, due to ref <-> byref reinterpretation), and then the rest would go into GC-untracked struct locals. OBJs could "obscure" the underlying TYP_REF local a little better (due to some copy block morphing inefficiencies) hence we see (and saw) this pattern with them more.

[SingleAccretion]

@dotnet/jit-contrib

[BruceForstall]

I'll go ahead and approve this; it fixes an existing regression.

I'm still uncomfortable with how LCL_FLD and GC tracking are currently interacting. It seems like LCL_FLD should never be of a GC type. Unless the JIT tracks individual struct local fields and reports the corresponding GC-typed fields as slots to the GC.

[SingleAccretion]

Credit to @BruceForstall for investigating the issue.

Fixes#79518.

No code diffs, but some GC info diffs that look like fixed GC holes. Some TP losses on 32 bit, payed for with #79194.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Credit to @BruceForstall for investigating the issue.

Fixes #79518.

No code diffs, but some GC info diffs that look like fixed GC holes.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[BruceForstall]

I understand STOREIND{ref}(LCL_VAR_ADDR{byref}[V21], ref value): V21 goes GC live at this location.

What about:

• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 2], ref value) => illegal, ref needs to be aligned
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value) (on 64-bit) => the 2nd field of a non-promoted struct type local V21 is a ref? Then liveness will mark the "entire" V21 live after this? But non-promoted structs are always GC untracked. So it seems like this should be illegal, too.

[SingleAccretion]

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?

It could also be a ref-typed local itself, or even byref-typed (since refs are a subset of byrefs). Or it could be a larger struct such as in your 3rd example.

Ultimately, we could lower away LCL_FLD_ADDR [0] to LCL_VAR_ADDR, but it does not seem like a good idea to me to make that mandatory. For example, it would only add code to this change, we would still need to handle LCL_FLD_ADDR in the tree life updater.

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value), Then liveness will mark the "entire" V21 live after this?

Most commonly, yes, except for dead stores.

As you note, the GC tracking would not be affected either way since the underlying local must be a struct hence GC-untracked else it is UB.

I am not sure why should it be illegal? One can write code like that today, and it will work as expected. Edit: I suppose you meant that it would never result in GC info updates. That it true.

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

[BruceForstall]

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking. for STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them. It seems now that more node types need to handle struct fields of GC type updates to the codegen-tracked GC info.

[SingleAccretion]

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking

The only change of significance was that we can now see LCL_FLD_ADDR [0] at a zero offset where previously we would have seen LCL_VAR_ADDR. Ultimately, this is a result of a number of changes, but since the two nodes have exactly identical semantics, it ought not to matter modulo bugs such as this one.

STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them.

That is not quite the case, STORE_LCL_FLD has been handling GC types for a very long time, it just doesn't matter a lot for it because we would convert all zero-offset, off-of-GC-local LCL_FLDs into LCL_VARs in most circumstances (notably, not always, due to ref <-> byref reinterpretation), and then the rest would go into GC-untracked struct locals. OBJs could "obscure" the underlying TYP_REF local a little better (due to some copy block morphing inefficiencies) hence we see (and saw) this pattern with them more.

[SingleAccretion]

@dotnet/jit-contrib

[BruceForstall]

I'll go ahead and approve this; it fixes an existing regression.

I'm still uncomfortable with how LCL_FLD and GC tracking are currently interacting. It seems like LCL_FLD should never be of a GC type. Unless the JIT tracks individual struct local fields and reports the corresponding GC-typed fields as slots to the GC.

[SingleAccretion]

Credit to @BruceForstall for investigating the issue.

Fixes#79518.

No code diffs, but some GC info diffs that look like fixed GC holes. Some TP losses on 32 bit, payed for with #79194.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Credit to @BruceForstall for investigating the issue.

Fixes #79518.

No code diffs, but some GC info diffs that look like fixed GC holes.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[BruceForstall]

I understand STOREIND{ref}(LCL_VAR_ADDR{byref}[V21], ref value): V21 goes GC live at this location.

What about:

• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 2], ref value) => illegal, ref needs to be aligned
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value) (on 64-bit) => the 2nd field of a non-promoted struct type local V21 is a ref? Then liveness will mark the "entire" V21 live after this? But non-promoted structs are always GC untracked. So it seems like this should be illegal, too.

[SingleAccretion]

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?

It could also be a ref-typed local itself, or even byref-typed (since refs are a subset of byrefs). Or it could be a larger struct such as in your 3rd example.

Ultimately, we could lower away LCL_FLD_ADDR [0] to LCL_VAR_ADDR, but it does not seem like a good idea to me to make that mandatory. For example, it would only add code to this change, we would still need to handle LCL_FLD_ADDR in the tree life updater.

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value), Then liveness will mark the "entire" V21 live after this?

Most commonly, yes, except for dead stores.

As you note, the GC tracking would not be affected either way since the underlying local must be a struct hence GC-untracked else it is UB.

I am not sure why should it be illegal? One can write code like that today, and it will work as expected. Edit: I suppose you meant that it would never result in GC info updates. That it true.

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

[BruceForstall]

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking. for STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them. It seems now that more node types need to handle struct fields of GC type updates to the codegen-tracked GC info.

[SingleAccretion]

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking

The only change of significance was that we can now see LCL_FLD_ADDR [0] at a zero offset where previously we would have seen LCL_VAR_ADDR. Ultimately, this is a result of a number of changes, but since the two nodes have exactly identical semantics, it ought not to matter modulo bugs such as this one.

STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them.

That is not quite the case, STORE_LCL_FLD has been handling GC types for a very long time, it just doesn't matter a lot for it because we would convert all zero-offset, off-of-GC-local LCL_FLDs into LCL_VARs in most circumstances (notably, not always, due to ref <-> byref reinterpretation), and then the rest would go into GC-untracked struct locals. OBJs could "obscure" the underlying TYP_REF local a little better (due to some copy block morphing inefficiencies) hence we see (and saw) this pattern with them more.

[SingleAccretion]

@dotnet/jit-contrib

[BruceForstall]

I'll go ahead and approve this; it fixes an existing regression.

I'm still uncomfortable with how LCL_FLD and GC tracking are currently interacting. It seems like LCL_FLD should never be of a GC type. Unless the JIT tracks individual struct local fields and reports the corresponding GC-typed fields as slots to the GC.

[SingleAccretion]

Credit to @BruceForstall for investigating the issue.

Fixes#79518.

No code diffs, but some GC info diffs that look like fixed GC holes. Some TP losses on 32 bit, payed for with #79194.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Credit to @BruceForstall for investigating the issue.

Fixes #79518.

No code diffs, but some GC info diffs that look like fixed GC holes.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[BruceForstall]

I understand STOREIND{ref}(LCL_VAR_ADDR{byref}[V21], ref value): V21 goes GC live at this location.

What about:

• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 2], ref value) => illegal, ref needs to be aligned
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value) (on 64-bit) => the 2nd field of a non-promoted struct type local V21 is a ref? Then liveness will mark the "entire" V21 live after this? But non-promoted structs are always GC untracked. So it seems like this should be illegal, too.

[SingleAccretion]

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?

It could also be a ref-typed local itself, or even byref-typed (since refs are a subset of byrefs). Or it could be a larger struct such as in your 3rd example.

Ultimately, we could lower away LCL_FLD_ADDR [0] to LCL_VAR_ADDR, but it does not seem like a good idea to me to make that mandatory. For example, it would only add code to this change, we would still need to handle LCL_FLD_ADDR in the tree life updater.

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value), Then liveness will mark the "entire" V21 live after this?

Most commonly, yes, except for dead stores.

As you note, the GC tracking would not be affected either way since the underlying local must be a struct hence GC-untracked else it is UB.

I am not sure why should it be illegal? One can write code like that today, and it will work as expected. Edit: I suppose you meant that it would never result in GC info updates. That it true.

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

[BruceForstall]

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking. for STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them. It seems now that more node types need to handle struct fields of GC type updates to the codegen-tracked GC info.

[SingleAccretion]

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking

The only change of significance was that we can now see LCL_FLD_ADDR [0] at a zero offset where previously we would have seen LCL_VAR_ADDR. Ultimately, this is a result of a number of changes, but since the two nodes have exactly identical semantics, it ought not to matter modulo bugs such as this one.

STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them.

That is not quite the case, STORE_LCL_FLD has been handling GC types for a very long time, it just doesn't matter a lot for it because we would convert all zero-offset, off-of-GC-local LCL_FLDs into LCL_VARs in most circumstances (notably, not always, due to ref <-> byref reinterpretation), and then the rest would go into GC-untracked struct locals. OBJs could "obscure" the underlying TYP_REF local a little better (due to some copy block morphing inefficiencies) hence we see (and saw) this pattern with them more.

[SingleAccretion]

@dotnet/jit-contrib

[BruceForstall]

I'll go ahead and approve this; it fixes an existing regression.

I'm still uncomfortable with how LCL_FLD and GC tracking are currently interacting. It seems like LCL_FLD should never be of a GC type. Unless the JIT tracks individual struct local fields and reports the corresponding GC-typed fields as slots to the GC.

[SingleAccretion]

Credit to @BruceForstall for investigating the issue.

Fixes#79518.

No code diffs, but some GC info diffs that look like fixed GC holes. Some TP losses on 32 bit, payed for with #79194.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Credit to @BruceForstall for investigating the issue.

Fixes #79518.

No code diffs, but some GC info diffs that look like fixed GC holes.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[BruceForstall]

I understand STOREIND{ref}(LCL_VAR_ADDR{byref}[V21], ref value): V21 goes GC live at this location.

What about:

• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 2], ref value) => illegal, ref needs to be aligned
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value) (on 64-bit) => the 2nd field of a non-promoted struct type local V21 is a ref? Then liveness will mark the "entire" V21 live after this? But non-promoted structs are always GC untracked. So it seems like this should be illegal, too.

[SingleAccretion]

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?

It could also be a ref-typed local itself, or even byref-typed (since refs are a subset of byrefs). Or it could be a larger struct such as in your 3rd example.

Ultimately, we could lower away LCL_FLD_ADDR [0] to LCL_VAR_ADDR, but it does not seem like a good idea to me to make that mandatory. For example, it would only add code to this change, we would still need to handle LCL_FLD_ADDR in the tree life updater.

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value), Then liveness will mark the "entire" V21 live after this?

Most commonly, yes, except for dead stores.

As you note, the GC tracking would not be affected either way since the underlying local must be a struct hence GC-untracked else it is UB.

I am not sure why should it be illegal? One can write code like that today, and it will work as expected. Edit: I suppose you meant that it would never result in GC info updates. That it true.

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

[BruceForstall]

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking. for STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them. It seems now that more node types need to handle struct fields of GC type updates to the codegen-tracked GC info.

[SingleAccretion]

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking

The only change of significance was that we can now see LCL_FLD_ADDR [0] at a zero offset where previously we would have seen LCL_VAR_ADDR. Ultimately, this is a result of a number of changes, but since the two nodes have exactly identical semantics, it ought not to matter modulo bugs such as this one.

STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them.

That is not quite the case, STORE_LCL_FLD has been handling GC types for a very long time, it just doesn't matter a lot for it because we would convert all zero-offset, off-of-GC-local LCL_FLDs into LCL_VARs in most circumstances (notably, not always, due to ref <-> byref reinterpretation), and then the rest would go into GC-untracked struct locals. OBJs could "obscure" the underlying TYP_REF local a little better (due to some copy block morphing inefficiencies) hence we see (and saw) this pattern with them more.

[SingleAccretion]

@dotnet/jit-contrib

[BruceForstall]

I'll go ahead and approve this; it fixes an existing regression.

I'm still uncomfortable with how LCL_FLD and GC tracking are currently interacting. It seems like LCL_FLD should never be of a GC type. Unless the JIT tracks individual struct local fields and reports the corresponding GC-typed fields as slots to the GC.

[SingleAccretion]

Credit to @BruceForstall for investigating the issue.

Fixes#79518.

No code diffs, but some GC info diffs that look like fixed GC holes. Some TP losses on 32 bit, payed for with #79194.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Credit to @BruceForstall for investigating the issue.

Fixes #79518.

No code diffs, but some GC info diffs that look like fixed GC holes.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[BruceForstall]

I understand STOREIND{ref}(LCL_VAR_ADDR{byref}[V21], ref value): V21 goes GC live at this location.

What about:

• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 2], ref value) => illegal, ref needs to be aligned
• STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value) (on 64-bit) => the 2nd field of a non-promoted struct type local V21 is a ref? Then liveness will mark the "entire" V21 live after this? But non-promoted structs are always GC untracked. So it seems like this should be illegal, too.

[SingleAccretion]

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 0], ref value) => why not morph LCL_FLD_ADDR{byref}[V21 + 0] to LCL_VAR_ADDR{byref}[V21]? Maybe this should only be legal for V21 a struct type that contains exactly one ref type member?

It could also be a ref-typed local itself, or even byref-typed (since refs are a subset of byrefs). Or it could be a larger struct such as in your 3rd example.

Ultimately, we could lower away LCL_FLD_ADDR [0] to LCL_VAR_ADDR, but it does not seem like a good idea to me to make that mandatory. For example, it would only add code to this change, we would still need to handle LCL_FLD_ADDR in the tree life updater.

STOREIND{ref}(LCL_FLD_ADDR{byref}[V21 + 8], ref value), Then liveness will mark the "entire" V21 live after this?

Most commonly, yes, except for dead stores.

As you note, the GC tracking would not be affected either way since the underlying local must be a struct hence GC-untracked else it is UB.

I am not sure why should it be illegal? One can write code like that today, and it will work as expected. Edit: I suppose you meant that it would never result in GC info updates. That it true.

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

[BruceForstall]

Really, we are not adding anything "new" here. STOREIND(LCL_FLD_ADDR[offs], ...) is equivalent to STORE_LCL_FLD([offs]...), which is the canonical form of this sort of store.

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking. for STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them. It seems now that more node types need to handle struct fields of GC type updates to the codegen-tracked GC info.

[SingleAccretion]

I think what I'm trying to understand is how the JIT IR has been changed recently w.r.t. struct field GC tracking

The only change of significance was that we can now see LCL_FLD_ADDR [0] at a zero offset where previously we would have seen LCL_VAR_ADDR. Ultimately, this is a result of a number of changes, but since the two nodes have exactly identical semantics, it ought not to matter modulo bugs such as this one.

STORE_LCL_FLD I don't think we ever allowed GC refs; they were always handled as STORE_OBJ, which knew how to handle them.

That is not quite the case, STORE_LCL_FLD has been handling GC types for a very long time, it just doesn't matter a lot for it because we would convert all zero-offset, off-of-GC-local LCL_FLDs into LCL_VARs in most circumstances (notably, not always, due to ref <-> byref reinterpretation), and then the rest would go into GC-untracked struct locals. OBJs could "obscure" the underlying TYP_REF local a little better (due to some copy block morphing inefficiencies) hence we see (and saw) this pattern with them more.

[SingleAccretion]

@dotnet/jit-contrib

[BruceForstall]

I'll go ahead and approve this; it fixes an existing regression.

I'm still uncomfortable with how LCL_FLD and GC tracking are currently interacting. It seems like LCL_FLD should never be of a GC type. Unless the JIT tracks individual struct local fields and reports the corresponding GC-typed fields as slots to the GC.