Remove GTF_GLOB_REF on a few more addressing nodes

[jakobbotsch]

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes#68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

[ghost]

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

Issue Details

---

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes #68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

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

[SingleAccretion]

Nit: could you keep the mention of the ADDR in the comment (here and in morph)?

It makes it stand out as the quirk this special handling is (and will help find places that need to be updated when ADDR is deleted).

[jakobbotsch]

I've readded the check for GT_ADDR, I don't believe we need to handle other cases anyway as they will just be handled when it's their turn in morph anyway.

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @kunalspathak, this is a fix for #68669

[kunalspathak]

I realize that we should also check for !varDsc->IsAddressExposed() below:

runtime/src/coreclr/jit/lclmorph.cpp

Line 687 in 6cc626f

if (varTypeIsStruct(varDsc) && varDsc->lvIsTemp)

Looking at the tree that you have in the repro, while visiting [000196] , we mark it as address-exposed and then while visiting [000198] we mark it as hidden buffer.

LocalAddressVisitor visiting statement:
STMT00044 ( INL14 @ 0x015[E-] ... ??? ) <- INL12 @ ??? <- INL09 @ 0x00B[E-] <- INLRT @ ???
[000198] S-C-G------- ▌ CALL void System.Reflection.TypeLoading.Ecma.MetadataExtensions.GetMethodDefinition
[000203] ------------ retbuf ├──▌ ADDR byref [000202] -------N---- │ └──▌ FIELD struct hackishFieldName
[000191] ------------ │ └──▌ ADDR byref [000192] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 [000200] n----------- arg1 ├──▌ OBJ struct<System.Reflection.Metadata.MethodDefinitionHandle, 4>
[000199] ------------ │ └──▌ ADDR byref [000193] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.Metadata.MethodDefinitionHandle, 4> V18 tmp16 [000196] --C-G------- arg2 └──▌ CALL ref System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder.get_Reader
[000194] ------------ this └──▌ ADDR byref [000195] -------N---- └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 Local V16 should not be enregistered because: it is address exposed
Local V16 should not be enregistered because: it is hidden buffer struct arg

[SingleAccretion]

What's the reason for the special handling for address-exposed locals?

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

[kunalspathak]

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed. But yes, it can be marked exposed later on.

[SingleAccretion]

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed.

Well, the reason we don't mark it exposed is because we can reason about the liveness now.

I suppose the reason for not marking it (RB) would would be to avoid looking at these locals down the line (i. e. save a little throughput).

[jakobbotsch]

I realize that we should also check for !varDsc->IsAddressExposed() below:

It seems weird to me to do that, there is no guarantee that we visited the address-exposing tree before we visit the call here. So this can never be done for correctness, as @SingleAccretion points out above.

[kunalspathak]

there is no guarantee that we visited the address-exposing tree before we visit the call here.

Agree

[kunalspathak]

LGTM

[jakobbotsch]

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes#68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

[ghost]

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

Issue Details

---

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes #68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

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

[SingleAccretion]

Nit: could you keep the mention of the ADDR in the comment (here and in morph)?

It makes it stand out as the quirk this special handling is (and will help find places that need to be updated when ADDR is deleted).

[jakobbotsch]

I've readded the check for GT_ADDR, I don't believe we need to handle other cases anyway as they will just be handled when it's their turn in morph anyway.

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @kunalspathak, this is a fix for #68669

[kunalspathak]

I realize that we should also check for !varDsc->IsAddressExposed() below:

runtime/src/coreclr/jit/lclmorph.cpp

Line 687 in 6cc626f

if (varTypeIsStruct(varDsc) && varDsc->lvIsTemp)

Looking at the tree that you have in the repro, while visiting [000196] , we mark it as address-exposed and then while visiting [000198] we mark it as hidden buffer.

LocalAddressVisitor visiting statement:
STMT00044 ( INL14 @ 0x015[E-] ... ??? ) <- INL12 @ ??? <- INL09 @ 0x00B[E-] <- INLRT @ ???
[000198] S-C-G------- ▌ CALL void System.Reflection.TypeLoading.Ecma.MetadataExtensions.GetMethodDefinition
[000203] ------------ retbuf ├──▌ ADDR byref [000202] -------N---- │ └──▌ FIELD struct hackishFieldName
[000191] ------------ │ └──▌ ADDR byref [000192] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 [000200] n----------- arg1 ├──▌ OBJ struct<System.Reflection.Metadata.MethodDefinitionHandle, 4>
[000199] ------------ │ └──▌ ADDR byref [000193] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.Metadata.MethodDefinitionHandle, 4> V18 tmp16 [000196] --C-G------- arg2 └──▌ CALL ref System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder.get_Reader
[000194] ------------ this └──▌ ADDR byref [000195] -------N---- └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 Local V16 should not be enregistered because: it is address exposed
Local V16 should not be enregistered because: it is hidden buffer struct arg

[SingleAccretion]

What's the reason for the special handling for address-exposed locals?

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

[kunalspathak]

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed. But yes, it can be marked exposed later on.

[SingleAccretion]

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed.

Well, the reason we don't mark it exposed is because we can reason about the liveness now.

I suppose the reason for not marking it (RB) would would be to avoid looking at these locals down the line (i. e. save a little throughput).

[jakobbotsch]

I realize that we should also check for !varDsc->IsAddressExposed() below:

It seems weird to me to do that, there is no guarantee that we visited the address-exposing tree before we visit the call here. So this can never be done for correctness, as @SingleAccretion points out above.

[kunalspathak]

there is no guarantee that we visited the address-exposing tree before we visit the call here.

Agree

[kunalspathak]

LGTM

[jakobbotsch]

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes#68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

[ghost]

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

Issue Details

---

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes #68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

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

[SingleAccretion]

Nit: could you keep the mention of the ADDR in the comment (here and in morph)?

It makes it stand out as the quirk this special handling is (and will help find places that need to be updated when ADDR is deleted).

[jakobbotsch]

I've readded the check for GT_ADDR, I don't believe we need to handle other cases anyway as they will just be handled when it's their turn in morph anyway.

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @kunalspathak, this is a fix for #68669

[kunalspathak]

I realize that we should also check for !varDsc->IsAddressExposed() below:

runtime/src/coreclr/jit/lclmorph.cpp

Line 687 in 6cc626f

if (varTypeIsStruct(varDsc) && varDsc->lvIsTemp)

Looking at the tree that you have in the repro, while visiting [000196] , we mark it as address-exposed and then while visiting [000198] we mark it as hidden buffer.

LocalAddressVisitor visiting statement:
STMT00044 ( INL14 @ 0x015[E-] ... ??? ) <- INL12 @ ??? <- INL09 @ 0x00B[E-] <- INLRT @ ???
[000198] S-C-G------- ▌ CALL void System.Reflection.TypeLoading.Ecma.MetadataExtensions.GetMethodDefinition
[000203] ------------ retbuf ├──▌ ADDR byref [000202] -------N---- │ └──▌ FIELD struct hackishFieldName
[000191] ------------ │ └──▌ ADDR byref [000192] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 [000200] n----------- arg1 ├──▌ OBJ struct<System.Reflection.Metadata.MethodDefinitionHandle, 4>
[000199] ------------ │ └──▌ ADDR byref [000193] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.Metadata.MethodDefinitionHandle, 4> V18 tmp16 [000196] --C-G------- arg2 └──▌ CALL ref System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder.get_Reader
[000194] ------------ this └──▌ ADDR byref [000195] -------N---- └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 Local V16 should not be enregistered because: it is address exposed
Local V16 should not be enregistered because: it is hidden buffer struct arg

[SingleAccretion]

What's the reason for the special handling for address-exposed locals?

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

[kunalspathak]

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed. But yes, it can be marked exposed later on.

[SingleAccretion]

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed.

Well, the reason we don't mark it exposed is because we can reason about the liveness now.

I suppose the reason for not marking it (RB) would would be to avoid looking at these locals down the line (i. e. save a little throughput).

[jakobbotsch]

I realize that we should also check for !varDsc->IsAddressExposed() below:

It seems weird to me to do that, there is no guarantee that we visited the address-exposing tree before we visit the call here. So this can never be done for correctness, as @SingleAccretion points out above.

[kunalspathak]

there is no guarantee that we visited the address-exposing tree before we visit the call here.

Agree

[kunalspathak]

LGTM

[jakobbotsch]

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes#68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

[ghost]

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

Issue Details

---

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes #68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

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

[SingleAccretion]

Nit: could you keep the mention of the ADDR in the comment (here and in morph)?

It makes it stand out as the quirk this special handling is (and will help find places that need to be updated when ADDR is deleted).

[jakobbotsch]

I've readded the check for GT_ADDR, I don't believe we need to handle other cases anyway as they will just be handled when it's their turn in morph anyway.

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @kunalspathak, this is a fix for #68669

[kunalspathak]

I realize that we should also check for !varDsc->IsAddressExposed() below:

runtime/src/coreclr/jit/lclmorph.cpp

Line 687 in 6cc626f

if (varTypeIsStruct(varDsc) && varDsc->lvIsTemp)

Looking at the tree that you have in the repro, while visiting [000196] , we mark it as address-exposed and then while visiting [000198] we mark it as hidden buffer.

LocalAddressVisitor visiting statement:
STMT00044 ( INL14 @ 0x015[E-] ... ??? ) <- INL12 @ ??? <- INL09 @ 0x00B[E-] <- INLRT @ ???
[000198] S-C-G------- ▌ CALL void System.Reflection.TypeLoading.Ecma.MetadataExtensions.GetMethodDefinition
[000203] ------------ retbuf ├──▌ ADDR byref [000202] -------N---- │ └──▌ FIELD struct hackishFieldName
[000191] ------------ │ └──▌ ADDR byref [000192] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 [000200] n----------- arg1 ├──▌ OBJ struct<System.Reflection.Metadata.MethodDefinitionHandle, 4>
[000199] ------------ │ └──▌ ADDR byref [000193] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.Metadata.MethodDefinitionHandle, 4> V18 tmp16 [000196] --C-G------- arg2 └──▌ CALL ref System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder.get_Reader
[000194] ------------ this └──▌ ADDR byref [000195] -------N---- └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 Local V16 should not be enregistered because: it is address exposed
Local V16 should not be enregistered because: it is hidden buffer struct arg

[SingleAccretion]

What's the reason for the special handling for address-exposed locals?

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

[kunalspathak]

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed. But yes, it can be marked exposed later on.

[SingleAccretion]

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed.

Well, the reason we don't mark it exposed is because we can reason about the liveness now.

I suppose the reason for not marking it (RB) would would be to avoid looking at these locals down the line (i. e. save a little throughput).

[jakobbotsch]

I realize that we should also check for !varDsc->IsAddressExposed() below:

It seems weird to me to do that, there is no guarantee that we visited the address-exposing tree before we visit the call here. So this can never be done for correctness, as @SingleAccretion points out above.

[kunalspathak]

there is no guarantee that we visited the address-exposing tree before we visit the call here.

Agree

[kunalspathak]

LGTM

[jakobbotsch]

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes#68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

[ghost]

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

Issue Details

---

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes #68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

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

[SingleAccretion]

Nit: could you keep the mention of the ADDR in the comment (here and in morph)?

It makes it stand out as the quirk this special handling is (and will help find places that need to be updated when ADDR is deleted).

[jakobbotsch]

I've readded the check for GT_ADDR, I don't believe we need to handle other cases anyway as they will just be handled when it's their turn in morph anyway.

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @kunalspathak, this is a fix for #68669

[kunalspathak]

I realize that we should also check for !varDsc->IsAddressExposed() below:

runtime/src/coreclr/jit/lclmorph.cpp

Line 687 in 6cc626f

if (varTypeIsStruct(varDsc) && varDsc->lvIsTemp)

Looking at the tree that you have in the repro, while visiting [000196] , we mark it as address-exposed and then while visiting [000198] we mark it as hidden buffer.

LocalAddressVisitor visiting statement:
STMT00044 ( INL14 @ 0x015[E-] ... ??? ) <- INL12 @ ??? <- INL09 @ 0x00B[E-] <- INLRT @ ???
[000198] S-C-G------- ▌ CALL void System.Reflection.TypeLoading.Ecma.MetadataExtensions.GetMethodDefinition
[000203] ------------ retbuf ├──▌ ADDR byref [000202] -------N---- │ └──▌ FIELD struct hackishFieldName
[000191] ------------ │ └──▌ ADDR byref [000192] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 [000200] n----------- arg1 ├──▌ OBJ struct<System.Reflection.Metadata.MethodDefinitionHandle, 4>
[000199] ------------ │ └──▌ ADDR byref [000193] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.Metadata.MethodDefinitionHandle, 4> V18 tmp16 [000196] --C-G------- arg2 └──▌ CALL ref System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder.get_Reader
[000194] ------------ this └──▌ ADDR byref [000195] -------N---- └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 Local V16 should not be enregistered because: it is address exposed
Local V16 should not be enregistered because: it is hidden buffer struct arg

[SingleAccretion]

What's the reason for the special handling for address-exposed locals?

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

[kunalspathak]

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed. But yes, it can be marked exposed later on.

[SingleAccretion]

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed.

Well, the reason we don't mark it exposed is because we can reason about the liveness now.

I suppose the reason for not marking it (RB) would would be to avoid looking at these locals down the line (i. e. save a little throughput).

[jakobbotsch]

I realize that we should also check for !varDsc->IsAddressExposed() below:

It seems weird to me to do that, there is no guarantee that we visited the address-exposing tree before we visit the call here. So this can never be done for correctness, as @SingleAccretion points out above.

[kunalspathak]

there is no guarantee that we visited the address-exposing tree before we visit the call here.

Agree

[kunalspathak]

LGTM

[jakobbotsch]

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes#68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

[ghost]

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

Issue Details

---

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes #68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

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

[SingleAccretion]

Nit: could you keep the mention of the ADDR in the comment (here and in morph)?

It makes it stand out as the quirk this special handling is (and will help find places that need to be updated when ADDR is deleted).

[jakobbotsch]

I've readded the check for GT_ADDR, I don't believe we need to handle other cases anyway as they will just be handled when it's their turn in morph anyway.

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @kunalspathak, this is a fix for #68669

[kunalspathak]

I realize that we should also check for !varDsc->IsAddressExposed() below:

runtime/src/coreclr/jit/lclmorph.cpp

Line 687 in 6cc626f

if (varTypeIsStruct(varDsc) && varDsc->lvIsTemp)

Looking at the tree that you have in the repro, while visiting [000196] , we mark it as address-exposed and then while visiting [000198] we mark it as hidden buffer.

LocalAddressVisitor visiting statement:
STMT00044 ( INL14 @ 0x015[E-] ... ??? ) <- INL12 @ ??? <- INL09 @ 0x00B[E-] <- INLRT @ ???
[000198] S-C-G------- ▌ CALL void System.Reflection.TypeLoading.Ecma.MetadataExtensions.GetMethodDefinition
[000203] ------------ retbuf ├──▌ ADDR byref [000202] -------N---- │ └──▌ FIELD struct hackishFieldName
[000191] ------------ │ └──▌ ADDR byref [000192] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 [000200] n----------- arg1 ├──▌ OBJ struct<System.Reflection.Metadata.MethodDefinitionHandle, 4>
[000199] ------------ │ └──▌ ADDR byref [000193] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.Metadata.MethodDefinitionHandle, 4> V18 tmp16 [000196] --C-G------- arg2 └──▌ CALL ref System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder.get_Reader
[000194] ------------ this └──▌ ADDR byref [000195] -------N---- └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 Local V16 should not be enregistered because: it is address exposed
Local V16 should not be enregistered because: it is hidden buffer struct arg

[SingleAccretion]

What's the reason for the special handling for address-exposed locals?

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

[kunalspathak]

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed. But yes, it can be marked exposed later on.

[SingleAccretion]

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed.

Well, the reason we don't mark it exposed is because we can reason about the liveness now.

I suppose the reason for not marking it (RB) would would be to avoid looking at these locals down the line (i. e. save a little throughput).

[jakobbotsch]

I realize that we should also check for !varDsc->IsAddressExposed() below:

It seems weird to me to do that, there is no guarantee that we visited the address-exposing tree before we visit the call here. So this can never be done for correctness, as @SingleAccretion points out above.

[kunalspathak]

there is no guarantee that we visited the address-exposing tree before we visit the call here.

Agree

[kunalspathak]

LGTM

[jakobbotsch]

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes#68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

[ghost]

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

Issue Details

---

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes #68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

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

[SingleAccretion]

Nit: could you keep the mention of the ADDR in the comment (here and in morph)?

It makes it stand out as the quirk this special handling is (and will help find places that need to be updated when ADDR is deleted).

[jakobbotsch]

I've readded the check for GT_ADDR, I don't believe we need to handle other cases anyway as they will just be handled when it's their turn in morph anyway.

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @kunalspathak, this is a fix for #68669

[kunalspathak]

I realize that we should also check for !varDsc->IsAddressExposed() below:

runtime/src/coreclr/jit/lclmorph.cpp

Line 687 in 6cc626f

if (varTypeIsStruct(varDsc) && varDsc->lvIsTemp)

Looking at the tree that you have in the repro, while visiting [000196] , we mark it as address-exposed and then while visiting [000198] we mark it as hidden buffer.

LocalAddressVisitor visiting statement:
STMT00044 ( INL14 @ 0x015[E-] ... ??? ) <- INL12 @ ??? <- INL09 @ 0x00B[E-] <- INLRT @ ???
[000198] S-C-G------- ▌ CALL void System.Reflection.TypeLoading.Ecma.MetadataExtensions.GetMethodDefinition
[000203] ------------ retbuf ├──▌ ADDR byref [000202] -------N---- │ └──▌ FIELD struct hackishFieldName
[000191] ------------ │ └──▌ ADDR byref [000192] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 [000200] n----------- arg1 ├──▌ OBJ struct<System.Reflection.Metadata.MethodDefinitionHandle, 4>
[000199] ------------ │ └──▌ ADDR byref [000193] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.Metadata.MethodDefinitionHandle, 4> V18 tmp16 [000196] --C-G------- arg2 └──▌ CALL ref System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder.get_Reader
[000194] ------------ this └──▌ ADDR byref [000195] -------N---- └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 Local V16 should not be enregistered because: it is address exposed
Local V16 should not be enregistered because: it is hidden buffer struct arg

[SingleAccretion]

What's the reason for the special handling for address-exposed locals?

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

[kunalspathak]

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed. But yes, it can be marked exposed later on.

[SingleAccretion]

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed.

Well, the reason we don't mark it exposed is because we can reason about the liveness now.

I suppose the reason for not marking it (RB) would would be to avoid looking at these locals down the line (i. e. save a little throughput).

[jakobbotsch]

I realize that we should also check for !varDsc->IsAddressExposed() below:

It seems weird to me to do that, there is no guarantee that we visited the address-exposing tree before we visit the call here. So this can never be done for correctness, as @SingleAccretion points out above.

[kunalspathak]

there is no guarantee that we visited the address-exposing tree before we visit the call here.

Agree

[kunalspathak]

LGTM

[jakobbotsch]

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes#68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

[ghost]

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

Issue Details

---

GTF_GLOB_REF is necessary on a GT_ADDR node only if the child actually
uses a tree with this flag as a value (and not as a location). This
removes the flag in a few more places by using IsLocalAddrExpr.

Fixes #68669

I've also improved the assertion for not breaking recognition of the retbuf.
I realized that in release builds even if we did introduce a temp, the noway_assert that I added would pass.

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

[SingleAccretion]

Nit: could you keep the mention of the ADDR in the comment (here and in morph)?

It makes it stand out as the quirk this special handling is (and will help find places that need to be updated when ADDR is deleted).

[jakobbotsch]

I've readded the check for GT_ADDR, I don't believe we need to handle other cases anyway as they will just be handled when it's their turn in morph anyway.

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @kunalspathak, this is a fix for #68669

[kunalspathak]

I realize that we should also check for !varDsc->IsAddressExposed() below:

runtime/src/coreclr/jit/lclmorph.cpp

Line 687 in 6cc626f

if (varTypeIsStruct(varDsc) && varDsc->lvIsTemp)

Looking at the tree that you have in the repro, while visiting [000196] , we mark it as address-exposed and then while visiting [000198] we mark it as hidden buffer.

LocalAddressVisitor visiting statement:
STMT00044 ( INL14 @ 0x015[E-] ... ??? ) <- INL12 @ ??? <- INL09 @ 0x00B[E-] <- INLRT @ ???
[000198] S-C-G------- ▌ CALL void System.Reflection.TypeLoading.Ecma.MetadataExtensions.GetMethodDefinition
[000203] ------------ retbuf ├──▌ ADDR byref [000202] -------N---- │ └──▌ FIELD struct hackishFieldName
[000191] ------------ │ └──▌ ADDR byref [000192] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 [000200] n----------- arg1 ├──▌ OBJ struct<System.Reflection.Metadata.MethodDefinitionHandle, 4>
[000199] ------------ │ └──▌ ADDR byref [000193] -------N---- │ └──▌ LCL_VAR struct<System.Reflection.Metadata.MethodDefinitionHandle, 4> V18 tmp16 [000196] --C-G------- arg2 └──▌ CALL ref System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder.get_Reader
[000194] ------------ this └──▌ ADDR byref [000195] -------N---- └──▌ LCL_VAR struct<System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder, 32> V16 tmp14 Local V16 should not be enregistered because: it is address exposed
Local V16 should not be enregistered because: it is hidden buffer struct arg

[SingleAccretion]

What's the reason for the special handling for address-exposed locals?

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

[kunalspathak]

(Noting that previously non-exposed locals can become exposed "later on", as late as morph in fact)

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed. But yes, it can be marked exposed later on.

[SingleAccretion]

Here we explicitly not mark something address exposed if something was marked as "hidden buffer", so probably we should avoid marking something "hidden buffer" if it is addressed exposed.

Well, the reason we don't mark it exposed is because we can reason about the liveness now.

I suppose the reason for not marking it (RB) would would be to avoid looking at these locals down the line (i. e. save a little throughput).

[jakobbotsch]

I realize that we should also check for !varDsc->IsAddressExposed() below:

It seems weird to me to do that, there is no guarantee that we visited the address-exposing tree before we visit the call here. So this can never be done for correctness, as @SingleAccretion points out above.

[kunalspathak]

there is no guarantee that we visited the address-exposing tree before we visit the call here.

Agree

[kunalspathak]

LGTM