JIT: Remove GT_NULLCHECK

[jakobbotsch]

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here. It comes with some TP cost, but not too much,
and I did some TP work in #71767 and #73919 that more than pays for this. We've seen frequent issues with keeping these flags up to date so I think it is worth it for the small cost in TP. I will remove these flags completely in a follow-up.

[ghost]

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

Issue Details

---

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here to see the TP diff. If it is too expensive (which it probably is) then I think the best way is to compute it in one of the full IR walks right before early prop, since keeping it up to date is even harder than it was before now.

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

[EgorBo]

consider doing the same for BBF_HAS_IDX_LEN - it also caused pain few times already

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jakobbotsch]

I've verified that the improvements seen in #68588 do not regress from this. @kunalspathak, did you have a simple example you were using there that I can double check the exact codegen for to make even more sure that it is not badly impacted by this change?

[jakobbotsch]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

As a general note I will keep PR as a draft until we snap for rc1.

[kunalspathak]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

Sorry, I missed this. I do not recall the exact example and this came to my notice while working on hoisting expressions out of multi-nested loop. But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

[jakobbotsch]

But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

Yeah, I verified that these benchmarks do not regress with the change, but I wasn't totally sure where the logic was kicking in for them. I guess I can go investigate in further detail.

[jakobbotsch]

Couple of things I need to check/address:

1. This is changing unused indirs to be emitted as cmp byte ptr [reg], al on xarch instead of the previous mov <some byte register>, byte ptr [reg]. Since null checks become unused indirs now it also changes those from cmp byte ptr [reg], bytereg to cmp byte ptr [reg], al. That might be affected by dependencies if rax was loaded recently. It would be nice to do a byte compare with the lower byte of esp, but that is not addressable on x86 (and requires rex prefix on x64). We can also emit test byte ptr [reg], 0 instead, but it is one byte larger than cmp byte ptr [reg], al.
2. While this allows certain indirs/nullchecks to be CSE'd together, there is still the consideration that unused indirs can hoisted regardless of the memory they are pointing at changing. Today, we will bail on hoisting due to IsTreeVNInvariant return false. Probably solvable in a relatively simple manner from hoisting.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[jakobbotsch]

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here. It comes with some TP cost, but not too much,
and I did some TP work in #71767 and #73919 that more than pays for this. We've seen frequent issues with keeping these flags up to date so I think it is worth it for the small cost in TP. I will remove these flags completely in a follow-up.

[ghost]

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

Issue Details

---

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here to see the TP diff. If it is too expensive (which it probably is) then I think the best way is to compute it in one of the full IR walks right before early prop, since keeping it up to date is even harder than it was before now.

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

[EgorBo]

consider doing the same for BBF_HAS_IDX_LEN - it also caused pain few times already

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jakobbotsch]

I've verified that the improvements seen in #68588 do not regress from this. @kunalspathak, did you have a simple example you were using there that I can double check the exact codegen for to make even more sure that it is not badly impacted by this change?

[jakobbotsch]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

As a general note I will keep PR as a draft until we snap for rc1.

[kunalspathak]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

Sorry, I missed this. I do not recall the exact example and this came to my notice while working on hoisting expressions out of multi-nested loop. But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

[jakobbotsch]

But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

Yeah, I verified that these benchmarks do not regress with the change, but I wasn't totally sure where the logic was kicking in for them. I guess I can go investigate in further detail.

[jakobbotsch]

Couple of things I need to check/address:

1. This is changing unused indirs to be emitted as cmp byte ptr [reg], al on xarch instead of the previous mov <some byte register>, byte ptr [reg]. Since null checks become unused indirs now it also changes those from cmp byte ptr [reg], bytereg to cmp byte ptr [reg], al. That might be affected by dependencies if rax was loaded recently. It would be nice to do a byte compare with the lower byte of esp, but that is not addressable on x86 (and requires rex prefix on x64). We can also emit test byte ptr [reg], 0 instead, but it is one byte larger than cmp byte ptr [reg], al.
2. While this allows certain indirs/nullchecks to be CSE'd together, there is still the consideration that unused indirs can hoisted regardless of the memory they are pointing at changing. Today, we will bail on hoisting due to IsTreeVNInvariant return false. Probably solvable in a relatively simple manner from hoisting.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[jakobbotsch]

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here. It comes with some TP cost, but not too much,
and I did some TP work in #71767 and #73919 that more than pays for this. We've seen frequent issues with keeping these flags up to date so I think it is worth it for the small cost in TP. I will remove these flags completely in a follow-up.

[ghost]

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

Issue Details

---

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here to see the TP diff. If it is too expensive (which it probably is) then I think the best way is to compute it in one of the full IR walks right before early prop, since keeping it up to date is even harder than it was before now.

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

[EgorBo]

consider doing the same for BBF_HAS_IDX_LEN - it also caused pain few times already

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jakobbotsch]

I've verified that the improvements seen in #68588 do not regress from this. @kunalspathak, did you have a simple example you were using there that I can double check the exact codegen for to make even more sure that it is not badly impacted by this change?

[jakobbotsch]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

As a general note I will keep PR as a draft until we snap for rc1.

[kunalspathak]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

Sorry, I missed this. I do not recall the exact example and this came to my notice while working on hoisting expressions out of multi-nested loop. But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

[jakobbotsch]

But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

Yeah, I verified that these benchmarks do not regress with the change, but I wasn't totally sure where the logic was kicking in for them. I guess I can go investigate in further detail.

[jakobbotsch]

Couple of things I need to check/address:

1. This is changing unused indirs to be emitted as cmp byte ptr [reg], al on xarch instead of the previous mov <some byte register>, byte ptr [reg]. Since null checks become unused indirs now it also changes those from cmp byte ptr [reg], bytereg to cmp byte ptr [reg], al. That might be affected by dependencies if rax was loaded recently. It would be nice to do a byte compare with the lower byte of esp, but that is not addressable on x86 (and requires rex prefix on x64). We can also emit test byte ptr [reg], 0 instead, but it is one byte larger than cmp byte ptr [reg], al.
2. While this allows certain indirs/nullchecks to be CSE'd together, there is still the consideration that unused indirs can hoisted regardless of the memory they are pointing at changing. Today, we will bail on hoisting due to IsTreeVNInvariant return false. Probably solvable in a relatively simple manner from hoisting.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[jakobbotsch]

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here. It comes with some TP cost, but not too much,
and I did some TP work in #71767 and #73919 that more than pays for this. We've seen frequent issues with keeping these flags up to date so I think it is worth it for the small cost in TP. I will remove these flags completely in a follow-up.

[ghost]

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

Issue Details

---

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here to see the TP diff. If it is too expensive (which it probably is) then I think the best way is to compute it in one of the full IR walks right before early prop, since keeping it up to date is even harder than it was before now.

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

[EgorBo]

consider doing the same for BBF_HAS_IDX_LEN - it also caused pain few times already

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jakobbotsch]

I've verified that the improvements seen in #68588 do not regress from this. @kunalspathak, did you have a simple example you were using there that I can double check the exact codegen for to make even more sure that it is not badly impacted by this change?

[jakobbotsch]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

As a general note I will keep PR as a draft until we snap for rc1.

[kunalspathak]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

Sorry, I missed this. I do not recall the exact example and this came to my notice while working on hoisting expressions out of multi-nested loop. But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

[jakobbotsch]

But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

Yeah, I verified that these benchmarks do not regress with the change, but I wasn't totally sure where the logic was kicking in for them. I guess I can go investigate in further detail.

[jakobbotsch]

Couple of things I need to check/address:

1. This is changing unused indirs to be emitted as cmp byte ptr [reg], al on xarch instead of the previous mov <some byte register>, byte ptr [reg]. Since null checks become unused indirs now it also changes those from cmp byte ptr [reg], bytereg to cmp byte ptr [reg], al. That might be affected by dependencies if rax was loaded recently. It would be nice to do a byte compare with the lower byte of esp, but that is not addressable on x86 (and requires rex prefix on x64). We can also emit test byte ptr [reg], 0 instead, but it is one byte larger than cmp byte ptr [reg], al.
2. While this allows certain indirs/nullchecks to be CSE'd together, there is still the consideration that unused indirs can hoisted regardless of the memory they are pointing at changing. Today, we will bail on hoisting due to IsTreeVNInvariant return false. Probably solvable in a relatively simple manner from hoisting.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[jakobbotsch]

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here. It comes with some TP cost, but not too much,
and I did some TP work in #71767 and #73919 that more than pays for this. We've seen frequent issues with keeping these flags up to date so I think it is worth it for the small cost in TP. I will remove these flags completely in a follow-up.

[ghost]

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

Issue Details

---

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here to see the TP diff. If it is too expensive (which it probably is) then I think the best way is to compute it in one of the full IR walks right before early prop, since keeping it up to date is even harder than it was before now.

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

[EgorBo]

consider doing the same for BBF_HAS_IDX_LEN - it also caused pain few times already

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jakobbotsch]

I've verified that the improvements seen in #68588 do not regress from this. @kunalspathak, did you have a simple example you were using there that I can double check the exact codegen for to make even more sure that it is not badly impacted by this change?

[jakobbotsch]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

As a general note I will keep PR as a draft until we snap for rc1.

[kunalspathak]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

Sorry, I missed this. I do not recall the exact example and this came to my notice while working on hoisting expressions out of multi-nested loop. But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

[jakobbotsch]

But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

Yeah, I verified that these benchmarks do not regress with the change, but I wasn't totally sure where the logic was kicking in for them. I guess I can go investigate in further detail.

[jakobbotsch]

Couple of things I need to check/address:

1. This is changing unused indirs to be emitted as cmp byte ptr [reg], al on xarch instead of the previous mov <some byte register>, byte ptr [reg]. Since null checks become unused indirs now it also changes those from cmp byte ptr [reg], bytereg to cmp byte ptr [reg], al. That might be affected by dependencies if rax was loaded recently. It would be nice to do a byte compare with the lower byte of esp, but that is not addressable on x86 (and requires rex prefix on x64). We can also emit test byte ptr [reg], 0 instead, but it is one byte larger than cmp byte ptr [reg], al.
2. While this allows certain indirs/nullchecks to be CSE'd together, there is still the consideration that unused indirs can hoisted regardless of the memory they are pointing at changing. Today, we will bail on hoisting due to IsTreeVNInvariant return false. Probably solvable in a relatively simple manner from hoisting.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[jakobbotsch]

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here. It comes with some TP cost, but not too much,
and I did some TP work in #71767 and #73919 that more than pays for this. We've seen frequent issues with keeping these flags up to date so I think it is worth it for the small cost in TP. I will remove these flags completely in a follow-up.

[ghost]

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

Issue Details

---

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here to see the TP diff. If it is too expensive (which it probably is) then I think the best way is to compute it in one of the full IR walks right before early prop, since keeping it up to date is even harder than it was before now.

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

[EgorBo]

consider doing the same for BBF_HAS_IDX_LEN - it also caused pain few times already

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jakobbotsch]

I've verified that the improvements seen in #68588 do not regress from this. @kunalspathak, did you have a simple example you were using there that I can double check the exact codegen for to make even more sure that it is not badly impacted by this change?

[jakobbotsch]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

As a general note I will keep PR as a draft until we snap for rc1.

[kunalspathak]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

Sorry, I missed this. I do not recall the exact example and this came to my notice while working on hoisting expressions out of multi-nested loop. But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

[jakobbotsch]

But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

Yeah, I verified that these benchmarks do not regress with the change, but I wasn't totally sure where the logic was kicking in for them. I guess I can go investigate in further detail.

[jakobbotsch]

Couple of things I need to check/address:

1. This is changing unused indirs to be emitted as cmp byte ptr [reg], al on xarch instead of the previous mov <some byte register>, byte ptr [reg]. Since null checks become unused indirs now it also changes those from cmp byte ptr [reg], bytereg to cmp byte ptr [reg], al. That might be affected by dependencies if rax was loaded recently. It would be nice to do a byte compare with the lower byte of esp, but that is not addressable on x86 (and requires rex prefix on x64). We can also emit test byte ptr [reg], 0 instead, but it is one byte larger than cmp byte ptr [reg], al.
2. While this allows certain indirs/nullchecks to be CSE'd together, there is still the consideration that unused indirs can hoisted regardless of the memory they are pointing at changing. Today, we will bail on hoisting due to IsTreeVNInvariant return false. Probably solvable in a relatively simple manner from hoisting.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[jakobbotsch]

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here. It comes with some TP cost, but not too much,
and I did some TP work in #71767 and #73919 that more than pays for this. We've seen frequent issues with keeping these flags up to date so I think it is worth it for the small cost in TP. I will remove these flags completely in a follow-up.

[ghost]

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

Issue Details

---

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here to see the TP diff. If it is too expensive (which it probably is) then I think the best way is to compute it in one of the full IR walks right before early prop, since keeping it up to date is even harder than it was before now.

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

[EgorBo]

consider doing the same for BBF_HAS_IDX_LEN - it also caused pain few times already

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jakobbotsch]

I've verified that the improvements seen in #68588 do not regress from this. @kunalspathak, did you have a simple example you were using there that I can double check the exact codegen for to make even more sure that it is not badly impacted by this change?

[jakobbotsch]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

As a general note I will keep PR as a draft until we snap for rc1.

[kunalspathak]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

Sorry, I missed this. I do not recall the exact example and this came to my notice while working on hoisting expressions out of multi-nested loop. But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

[jakobbotsch]

But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

Yeah, I verified that these benchmarks do not regress with the change, but I wasn't totally sure where the logic was kicking in for them. I guess I can go investigate in further detail.

[jakobbotsch]

Couple of things I need to check/address:

1. This is changing unused indirs to be emitted as cmp byte ptr [reg], al on xarch instead of the previous mov <some byte register>, byte ptr [reg]. Since null checks become unused indirs now it also changes those from cmp byte ptr [reg], bytereg to cmp byte ptr [reg], al. That might be affected by dependencies if rax was loaded recently. It would be nice to do a byte compare with the lower byte of esp, but that is not addressable on x86 (and requires rex prefix on x64). We can also emit test byte ptr [reg], 0 instead, but it is one byte larger than cmp byte ptr [reg], al.
2. While this allows certain indirs/nullchecks to be CSE'd together, there is still the consideration that unused indirs can hoisted regardless of the memory they are pointing at changing. Today, we will bail on hoisting due to IsTreeVNInvariant return false. Probably solvable in a relatively simple manner from hoisting.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[jakobbotsch]

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here. It comes with some TP cost, but not too much,
and I did some TP work in #71767 and #73919 that more than pays for this. We've seen frequent issues with keeping these flags up to date so I think it is worth it for the small cost in TP. I will remove these flags completely in a follow-up.

[ghost]

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

Issue Details

---

Previously the JIT had two ways of representing null checks, either
through GT_NULLCHECK or through any indirection node with an unused
value. This is somewhat problematic since early prop supports only the
former while CSE supports only the latter, so converting between them
always results in improvements and regressions.

To alleviate the problem remove GT_NULLCHECK entirely and switch to
unused indirections to represent null checks. Add the necessary support
for efficient codegen for these to the backend, and add support for
early prop to recognize and fold these away.

I am also removing the checks for the BBF_HAS_NULLCHECK flags here to see the TP diff. If it is too expensive (which it probably is) then I think the best way is to compute it in one of the full IR walks right before early prop, since keeping it up to date is even harder than it was before now.

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

[EgorBo]

consider doing the same for BBF_HAS_IDX_LEN - it also caused pain few times already

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jakobbotsch]

I've verified that the improvements seen in #68588 do not regress from this. @kunalspathak, did you have a simple example you were using there that I can double check the exact codegen for to make even more sure that it is not badly impacted by this change?

[jakobbotsch]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

As a general note I will keep PR as a draft until we snap for rc1.

[kunalspathak]

ping @kunalspathak, do you remember what case you were using to evaluate #68588?

Sorry, I missed this. I do not recall the exact example and this came to my notice while working on hoisting expressions out of multi-nested loop. But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

[jakobbotsch]

But looking at the improvements (e.g. dotnet/perf-autofiling-issues#5096), you can try one of that example to see.

Yeah, I verified that these benchmarks do not regress with the change, but I wasn't totally sure where the logic was kicking in for them. I guess I can go investigate in further detail.

[jakobbotsch]

Couple of things I need to check/address:

1. This is changing unused indirs to be emitted as cmp byte ptr [reg], al on xarch instead of the previous mov <some byte register>, byte ptr [reg]. Since null checks become unused indirs now it also changes those from cmp byte ptr [reg], bytereg to cmp byte ptr [reg], al. That might be affected by dependencies if rax was loaded recently. It would be nice to do a byte compare with the lower byte of esp, but that is not addressable on x86 (and requires rex prefix on x64). We can also emit test byte ptr [reg], 0 instead, but it is one byte larger than cmp byte ptr [reg], al.
2. While this allows certain indirs/nullchecks to be CSE'd together, there is still the consideration that unused indirs can hoisted regardless of the memory they are pointing at changing. Today, we will bail on hoisting due to IsTreeVNInvariant return false. Probably solvable in a relatively simple manner from hoisting.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.