Fix JitOptRepeat around shared constants

[EgorBo]

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node (0x7ffb48d0ee44 + 300 = 0x7ffb48d0ef70):

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a shared base constant (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have a VN that has ICON flag encoded into it. However, we completely erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node will no longer have an ICON flag after 2nd pass of VN.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

A test case where it easily reproduces: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/Runtime_70790/Runtime_70790.cs (s_intType is folded to a constant, also, note - 300 there).

[ghost]

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

Issue Details

---

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node:

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a base (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have VN that has ICON flag encoded into it. However, we erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node no longer has icon flag.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

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

[EgorBo]

@dotnet/jit-contrib @AndyAyersMS@BruceForstall PTAL, fixes an assert seen in SPMI for JitOptRepeat

[AndyAyersMS]

I wonder what else we could defer until the last pass.

[BruceForstall]

CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat. Specifically, it means that the results of shared constant CSE can't be further optimized.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

[EgorBo]

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat.

Do you have a better option in mind? We basically destroy handles at the first iteration converting them into randCns1 + randCns2 so optimizations at next iterations no longer can benefit from knowing what exactly constants are representing. We probably can do it if we find a way to preserve VN from last iteration somehow, otherwise next iteration of VN would assign a non-ICON VN to it.
PS: It's not a correctness fix, it's for better diffs.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

Ah ok, didn't see that, closing (although, I don't see how it fixes the test case I mentioned)

[EgorBo]

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node (0x7ffb48d0ee44 + 300 = 0x7ffb48d0ef70):

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a shared base constant (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have a VN that has ICON flag encoded into it. However, we completely erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node will no longer have an ICON flag after 2nd pass of VN.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

A test case where it easily reproduces: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/Runtime_70790/Runtime_70790.cs (s_intType is folded to a constant, also, note - 300 there).

[ghost]

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

Issue Details

---

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node:

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a base (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have VN that has ICON flag encoded into it. However, we erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node no longer has icon flag.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

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

[EgorBo]

@dotnet/jit-contrib @AndyAyersMS@BruceForstall PTAL, fixes an assert seen in SPMI for JitOptRepeat

[AndyAyersMS]

I wonder what else we could defer until the last pass.

[BruceForstall]

CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat. Specifically, it means that the results of shared constant CSE can't be further optimized.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

[EgorBo]

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat.

Do you have a better option in mind? We basically destroy handles at the first iteration converting them into randCns1 + randCns2 so optimizations at next iterations no longer can benefit from knowing what exactly constants are representing. We probably can do it if we find a way to preserve VN from last iteration somehow, otherwise next iteration of VN would assign a non-ICON VN to it.
PS: It's not a correctness fix, it's for better diffs.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

Ah ok, didn't see that, closing (although, I don't see how it fixes the test case I mentioned)

[EgorBo]

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node (0x7ffb48d0ee44 + 300 = 0x7ffb48d0ef70):

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a shared base constant (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have a VN that has ICON flag encoded into it. However, we completely erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node will no longer have an ICON flag after 2nd pass of VN.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

A test case where it easily reproduces: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/Runtime_70790/Runtime_70790.cs (s_intType is folded to a constant, also, note - 300 there).

[ghost]

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

Issue Details

---

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node:

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a base (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have VN that has ICON flag encoded into it. However, we erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node no longer has icon flag.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

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

[EgorBo]

@dotnet/jit-contrib @AndyAyersMS@BruceForstall PTAL, fixes an assert seen in SPMI for JitOptRepeat

[AndyAyersMS]

I wonder what else we could defer until the last pass.

[BruceForstall]

CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat. Specifically, it means that the results of shared constant CSE can't be further optimized.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

[EgorBo]

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat.

Do you have a better option in mind? We basically destroy handles at the first iteration converting them into randCns1 + randCns2 so optimizations at next iterations no longer can benefit from knowing what exactly constants are representing. We probably can do it if we find a way to preserve VN from last iteration somehow, otherwise next iteration of VN would assign a non-ICON VN to it.
PS: It's not a correctness fix, it's for better diffs.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

Ah ok, didn't see that, closing (although, I don't see how it fixes the test case I mentioned)

[EgorBo]

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node (0x7ffb48d0ee44 + 300 = 0x7ffb48d0ef70):

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a shared base constant (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have a VN that has ICON flag encoded into it. However, we completely erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node will no longer have an ICON flag after 2nd pass of VN.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

A test case where it easily reproduces: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/Runtime_70790/Runtime_70790.cs (s_intType is folded to a constant, also, note - 300 there).

[ghost]

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

Issue Details

---

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node:

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a base (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have VN that has ICON flag encoded into it. However, we erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node no longer has icon flag.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

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

[EgorBo]

@dotnet/jit-contrib @AndyAyersMS@BruceForstall PTAL, fixes an assert seen in SPMI for JitOptRepeat

[AndyAyersMS]

I wonder what else we could defer until the last pass.

[BruceForstall]

CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat. Specifically, it means that the results of shared constant CSE can't be further optimized.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

[EgorBo]

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat.

Do you have a better option in mind? We basically destroy handles at the first iteration converting them into randCns1 + randCns2 so optimizations at next iterations no longer can benefit from knowing what exactly constants are representing. We probably can do it if we find a way to preserve VN from last iteration somehow, otherwise next iteration of VN would assign a non-ICON VN to it.
PS: It's not a correctness fix, it's for better diffs.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

Ah ok, didn't see that, closing (although, I don't see how it fixes the test case I mentioned)

[EgorBo]

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node (0x7ffb48d0ee44 + 300 = 0x7ffb48d0ef70):

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a shared base constant (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have a VN that has ICON flag encoded into it. However, we completely erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node will no longer have an ICON flag after 2nd pass of VN.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

A test case where it easily reproduces: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/Runtime_70790/Runtime_70790.cs (s_intType is folded to a constant, also, note - 300 there).

[ghost]

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

Issue Details

---

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node:

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a base (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have VN that has ICON flag encoded into it. However, we erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node no longer has icon flag.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

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

[EgorBo]

@dotnet/jit-contrib @AndyAyersMS@BruceForstall PTAL, fixes an assert seen in SPMI for JitOptRepeat

[AndyAyersMS]

I wonder what else we could defer until the last pass.

[BruceForstall]

CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat. Specifically, it means that the results of shared constant CSE can't be further optimized.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

[EgorBo]

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat.

Do you have a better option in mind? We basically destroy handles at the first iteration converting them into randCns1 + randCns2 so optimizations at next iterations no longer can benefit from knowing what exactly constants are representing. We probably can do it if we find a way to preserve VN from last iteration somehow, otherwise next iteration of VN would assign a non-ICON VN to it.
PS: It's not a correctness fix, it's for better diffs.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

Ah ok, didn't see that, closing (although, I don't see how it fixes the test case I mentioned)

[EgorBo]

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node (0x7ffb48d0ee44 + 300 = 0x7ffb48d0ef70):

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a shared base constant (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have a VN that has ICON flag encoded into it. However, we completely erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node will no longer have an ICON flag after 2nd pass of VN.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

A test case where it easily reproduces: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/Runtime_70790/Runtime_70790.cs (s_intType is folded to a constant, also, note - 300 there).

[ghost]

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

Issue Details

---

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node:

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a base (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have VN that has ICON flag encoded into it. However, we erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node no longer has icon flag.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

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

[EgorBo]

@dotnet/jit-contrib @AndyAyersMS@BruceForstall PTAL, fixes an assert seen in SPMI for JitOptRepeat

[AndyAyersMS]

I wonder what else we could defer until the last pass.

[BruceForstall]

CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat. Specifically, it means that the results of shared constant CSE can't be further optimized.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

[EgorBo]

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat.

Do you have a better option in mind? We basically destroy handles at the first iteration converting them into randCns1 + randCns2 so optimizations at next iterations no longer can benefit from knowing what exactly constants are representing. We probably can do it if we find a way to preserve VN from last iteration somehow, otherwise next iteration of VN would assign a non-ICON VN to it.
PS: It's not a correctness fix, it's for better diffs.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

Ah ok, didn't see that, closing (although, I don't see how it fixes the test case I mentioned)

[EgorBo]

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node (0x7ffb48d0ee44 + 300 = 0x7ffb48d0ef70):

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a shared base constant (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have a VN that has ICON flag encoded into it. However, we completely erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node will no longer have an ICON flag after 2nd pass of VN.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

A test case where it easily reproduces: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/Runtime_70790/Runtime_70790.cs (s_intType is folded to a constant, also, note - 300 there).

[ghost]

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

Issue Details

---

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node:

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a base (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have VN that has ICON flag encoded into it. However, we erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node no longer has icon flag.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

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

[EgorBo]

@dotnet/jit-contrib @AndyAyersMS@BruceForstall PTAL, fixes an assert seen in SPMI for JitOptRepeat

[AndyAyersMS]

I wonder what else we could defer until the last pass.

[BruceForstall]

CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat. Specifically, it means that the results of shared constant CSE can't be further optimized.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

[EgorBo]

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat.

Do you have a better option in mind? We basically destroy handles at the first iteration converting them into randCns1 + randCns2 so optimizations at next iterations no longer can benefit from knowing what exactly constants are representing. We probably can do it if we find a way to preserve VN from last iteration somehow, otherwise next iteration of VN would assign a non-ICON VN to it.
PS: It's not a correctness fix, it's for better diffs.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

Ah ok, didn't see that, closing (although, I don't see how it fixes the test case I mentioned)

[EgorBo]

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node (0x7ffb48d0ee44 + 300 = 0x7ffb48d0ef70):

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a shared base constant (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have a VN that has ICON flag encoded into it. However, we completely erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node will no longer have an ICON flag after 2nd pass of VN.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

A test case where it easily reproduces: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/Runtime_70790/Runtime_70790.cs (s_intType is folded to a constant, also, note - 300 there).

[ghost]

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

Issue Details

---

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long 0x7ffb48d0ef70 class $1c0

to an ADD node:

\--* COMMA long $1c0
+--* STORE_LCL_VAR long V03 cse0 d:1 $VN.Void
| \--* CNS_INT long 0x7ffb48d0ee44 $240
\--* ADD long $1c0
+--* LCL_VAR long V03 cse0 u:1 $240
\--* CNS_INT long 300

where 0x7ffb48d0ee44 is picked as a base (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have VN that has ICON flag encoded into it. However, we erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node no longer has icon flag.

There are two fixes:

1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

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

[EgorBo]

@dotnet/jit-contrib @AndyAyersMS@BruceForstall PTAL, fixes an assert seen in SPMI for JitOptRepeat

[AndyAyersMS]

I wonder what else we could defer until the last pass.

[BruceForstall]

CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat. Specifically, it means that the results of shared constant CSE can't be further optimized.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

[EgorBo]

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat.

Do you have a better option in mind? We basically destroy handles at the first iteration converting them into randCns1 + randCns2 so optimizations at next iterations no longer can benefit from knowing what exactly constants are representing. We probably can do it if we find a way to preserve VN from last iteration somehow, otherwise next iteration of VN would assign a non-ICON VN to it.
PS: It's not a correctness fix, it's for better diffs.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

Ah ok, didn't see that, closing (although, I don't see how it fixes the test case I mentioned)