Add more checking to GenTreeArrAddr::ParseArrayAddress()

[BruceForstall]

If the ARR_ADDR node doesn't make sense, namely if the array offset does not appear to be in the array, then bail.

[BruceForstall]

Extracted from #94250

[BruceForstall]

There are a few diffs in ILGEN tests that construct array index expressions with negative indices.

[BruceForstall]

With JitOptRepeat we end up with the following situation.

We hit the assert:

Assertion failed '(constIndexOffset % elemSize) == 0'

(e.g.,

System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1+StateMachineBox`1[int,System.IO.Stream+<ReadAtLeastAsyncCore>d__46]:System.Threading.Tasks.Sources.IValueTaskSource.GetResult(short):this'

We start with:

[000895] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000894] ----------- \--* ADD byref
[000885] ----------- +--* LCL_VAR ref V56 tmp54
[000893] ----------- \--* ADD long
[000891] ----------- +--* MUL long
[000889] ---------U- | +--* CAST long <- uint
[000886] ----------- | | \--* LCL_VAR int V14 tmp12
[000890] -------N--- | \--* CNS_INT long 128
[000892] ----------- \--* CNS_INT long 16

After optimization, this becomes:

N018 ( 2, 2) [000895] -A---O----- \--* ARR_ADDR byref Internal.PaddedReference[] $641
N017 ( 2, 2) [000894] -A-----N--- \--* ADD byref $199
N007 ( 1, 1) [000885] ----------- +--* LCL_VAR ref V56 tmp54 u:1 (last use) $84
N016 ( 7, 10) [000995] -A--------- \--* COMMA long $687
N014 ( 6, 9) [000993] DA--------- +--* STORE_LCL_VAR long V62 cse3 d:1 $VN.Void
N013 ( 6, 9) [000893] ----------- | \--* ADD long $687
N011 ( 4, 6) [000891] ----------- | +--* LSH long $686
N009 ( 2, 3) [000889] ---------U- | | +--* CAST long <- uint $685
N008 ( 1, 1) [000886] ----------- | | | \--* LCL_VAR int V14 tmp12 u:1 $1f5
N010 ( 1, 2) [000890] -------N--- | | \--* CNS_INT long 7 $14e
N012 ( 1, 2) [000892] ----------- | \--* CNS_INT long 16 $146
N015 ( 1, 1) [000994] ----------- \--* LCL_VAR long V62 cse3 u:1 $687

On the next iteration of JitOptRepeat, the CSE injected COMMA is not parseable by GenTreeArrAddr::ParseArrayAddress. We could add tunnel through for this CSE def.

However, it might be harder for the CSE use. In that case, we start with something similar:

[000941] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000940] ----------- \--* ADD byref
[000931] ----------- +--* LCL_VAR ref V57 tmp55
[000939] ----------- \--* ADD long
[000937] ----------- +--* MUL long
[000935] ---------U- | +--* CAST long <- uint
[000932] ----------- | | \--* LCL_VAR int V14 tmp12 (last use)
[000936] -------N--- | \--* CNS_INT long 128
[000938] ----------- \--* CNS_INT long 16

and, after CSE, have:

N006 ( 2, 2) [000941] -----O-N--- \--* ARR_ADDR byref Internal.PaddedReference[]
N005 ( 2, 2) [000940] -------N--- \--* ADD byref
N003 ( 1, 1) [000931] ----------- +--* LCL_VAR ref V57 tmp55 u:1 (last use)
N004 ( 1, 1) [000996] ----------- \--* LCL_VAR long V62 cse3 u:1 (last use)

This would require parsing the CSE value number for [000996], not the tree.

Since this is VN iteration #2, I'm not sure we lose anything by assigning the ARR_ADDR a new, unique value number when we fail to parse the tree: we've already optimized it once.

[BruceForstall]

@jakobbotsch PTAL
cc @dotnet/jit-contrib

[BruceForstall]

It's interesting that there are diffs on x86, apparently because the offset of data of an array from the base of an array is 8 (it is 16 on 64-bit). This happens to be the same size as a pointer and a double. I see cases where we start with (loc5 - 1) * 8 + 8 which morph transforms to loc5 * 8 - 8 + 8 => loc5 * 8, thus we no longer have an "offset" into the array data (and at runtime loc5 better be > 0, which is checked by the bounds check).

The result is that we give the ARR_ADDR a new, unique VN and we lose some CSEs.

[BruceForstall]

GitHub is confused; all the tests passed.

Diffsas described

[BruceForstall]

If the ARR_ADDR node doesn't make sense, namely if the array offset does not appear to be in the array, then bail.

[BruceForstall]

Extracted from #94250

[BruceForstall]

There are a few diffs in ILGEN tests that construct array index expressions with negative indices.

[BruceForstall]

With JitOptRepeat we end up with the following situation.

We hit the assert:

Assertion failed '(constIndexOffset % elemSize) == 0'

(e.g.,

System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1+StateMachineBox`1[int,System.IO.Stream+<ReadAtLeastAsyncCore>d__46]:System.Threading.Tasks.Sources.IValueTaskSource.GetResult(short):this'

We start with:

[000895] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000894] ----------- \--* ADD byref
[000885] ----------- +--* LCL_VAR ref V56 tmp54
[000893] ----------- \--* ADD long
[000891] ----------- +--* MUL long
[000889] ---------U- | +--* CAST long <- uint
[000886] ----------- | | \--* LCL_VAR int V14 tmp12
[000890] -------N--- | \--* CNS_INT long 128
[000892] ----------- \--* CNS_INT long 16

After optimization, this becomes:

N018 ( 2, 2) [000895] -A---O----- \--* ARR_ADDR byref Internal.PaddedReference[] $641
N017 ( 2, 2) [000894] -A-----N--- \--* ADD byref $199
N007 ( 1, 1) [000885] ----------- +--* LCL_VAR ref V56 tmp54 u:1 (last use) $84
N016 ( 7, 10) [000995] -A--------- \--* COMMA long $687
N014 ( 6, 9) [000993] DA--------- +--* STORE_LCL_VAR long V62 cse3 d:1 $VN.Void
N013 ( 6, 9) [000893] ----------- | \--* ADD long $687
N011 ( 4, 6) [000891] ----------- | +--* LSH long $686
N009 ( 2, 3) [000889] ---------U- | | +--* CAST long <- uint $685
N008 ( 1, 1) [000886] ----------- | | | \--* LCL_VAR int V14 tmp12 u:1 $1f5
N010 ( 1, 2) [000890] -------N--- | | \--* CNS_INT long 7 $14e
N012 ( 1, 2) [000892] ----------- | \--* CNS_INT long 16 $146
N015 ( 1, 1) [000994] ----------- \--* LCL_VAR long V62 cse3 u:1 $687

On the next iteration of JitOptRepeat, the CSE injected COMMA is not parseable by GenTreeArrAddr::ParseArrayAddress. We could add tunnel through for this CSE def.

However, it might be harder for the CSE use. In that case, we start with something similar:

[000941] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000940] ----------- \--* ADD byref
[000931] ----------- +--* LCL_VAR ref V57 tmp55
[000939] ----------- \--* ADD long
[000937] ----------- +--* MUL long
[000935] ---------U- | +--* CAST long <- uint
[000932] ----------- | | \--* LCL_VAR int V14 tmp12 (last use)
[000936] -------N--- | \--* CNS_INT long 128
[000938] ----------- \--* CNS_INT long 16

and, after CSE, have:

N006 ( 2, 2) [000941] -----O-N--- \--* ARR_ADDR byref Internal.PaddedReference[]
N005 ( 2, 2) [000940] -------N--- \--* ADD byref
N003 ( 1, 1) [000931] ----------- +--* LCL_VAR ref V57 tmp55 u:1 (last use)
N004 ( 1, 1) [000996] ----------- \--* LCL_VAR long V62 cse3 u:1 (last use)

This would require parsing the CSE value number for [000996], not the tree.

Since this is VN iteration #2, I'm not sure we lose anything by assigning the ARR_ADDR a new, unique value number when we fail to parse the tree: we've already optimized it once.

[BruceForstall]

@jakobbotsch PTAL
cc @dotnet/jit-contrib

[BruceForstall]

It's interesting that there are diffs on x86, apparently because the offset of data of an array from the base of an array is 8 (it is 16 on 64-bit). This happens to be the same size as a pointer and a double. I see cases where we start with (loc5 - 1) * 8 + 8 which morph transforms to loc5 * 8 - 8 + 8 => loc5 * 8, thus we no longer have an "offset" into the array data (and at runtime loc5 better be > 0, which is checked by the bounds check).

The result is that we give the ARR_ADDR a new, unique VN and we lose some CSEs.

[BruceForstall]

GitHub is confused; all the tests passed.

Diffsas described

[BruceForstall]

If the ARR_ADDR node doesn't make sense, namely if the array offset does not appear to be in the array, then bail.

[BruceForstall]

Extracted from #94250

[BruceForstall]

There are a few diffs in ILGEN tests that construct array index expressions with negative indices.

[BruceForstall]

With JitOptRepeat we end up with the following situation.

We hit the assert:

Assertion failed '(constIndexOffset % elemSize) == 0'

(e.g.,

System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1+StateMachineBox`1[int,System.IO.Stream+<ReadAtLeastAsyncCore>d__46]:System.Threading.Tasks.Sources.IValueTaskSource.GetResult(short):this'

We start with:

[000895] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000894] ----------- \--* ADD byref
[000885] ----------- +--* LCL_VAR ref V56 tmp54
[000893] ----------- \--* ADD long
[000891] ----------- +--* MUL long
[000889] ---------U- | +--* CAST long <- uint
[000886] ----------- | | \--* LCL_VAR int V14 tmp12
[000890] -------N--- | \--* CNS_INT long 128
[000892] ----------- \--* CNS_INT long 16

After optimization, this becomes:

N018 ( 2, 2) [000895] -A---O----- \--* ARR_ADDR byref Internal.PaddedReference[] $641
N017 ( 2, 2) [000894] -A-----N--- \--* ADD byref $199
N007 ( 1, 1) [000885] ----------- +--* LCL_VAR ref V56 tmp54 u:1 (last use) $84
N016 ( 7, 10) [000995] -A--------- \--* COMMA long $687
N014 ( 6, 9) [000993] DA--------- +--* STORE_LCL_VAR long V62 cse3 d:1 $VN.Void
N013 ( 6, 9) [000893] ----------- | \--* ADD long $687
N011 ( 4, 6) [000891] ----------- | +--* LSH long $686
N009 ( 2, 3) [000889] ---------U- | | +--* CAST long <- uint $685
N008 ( 1, 1) [000886] ----------- | | | \--* LCL_VAR int V14 tmp12 u:1 $1f5
N010 ( 1, 2) [000890] -------N--- | | \--* CNS_INT long 7 $14e
N012 ( 1, 2) [000892] ----------- | \--* CNS_INT long 16 $146
N015 ( 1, 1) [000994] ----------- \--* LCL_VAR long V62 cse3 u:1 $687

On the next iteration of JitOptRepeat, the CSE injected COMMA is not parseable by GenTreeArrAddr::ParseArrayAddress. We could add tunnel through for this CSE def.

However, it might be harder for the CSE use. In that case, we start with something similar:

[000941] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000940] ----------- \--* ADD byref
[000931] ----------- +--* LCL_VAR ref V57 tmp55
[000939] ----------- \--* ADD long
[000937] ----------- +--* MUL long
[000935] ---------U- | +--* CAST long <- uint
[000932] ----------- | | \--* LCL_VAR int V14 tmp12 (last use)
[000936] -------N--- | \--* CNS_INT long 128
[000938] ----------- \--* CNS_INT long 16

and, after CSE, have:

N006 ( 2, 2) [000941] -----O-N--- \--* ARR_ADDR byref Internal.PaddedReference[]
N005 ( 2, 2) [000940] -------N--- \--* ADD byref
N003 ( 1, 1) [000931] ----------- +--* LCL_VAR ref V57 tmp55 u:1 (last use)
N004 ( 1, 1) [000996] ----------- \--* LCL_VAR long V62 cse3 u:1 (last use)

This would require parsing the CSE value number for [000996], not the tree.

Since this is VN iteration #2, I'm not sure we lose anything by assigning the ARR_ADDR a new, unique value number when we fail to parse the tree: we've already optimized it once.

[BruceForstall]

@jakobbotsch PTAL
cc @dotnet/jit-contrib

[BruceForstall]

It's interesting that there are diffs on x86, apparently because the offset of data of an array from the base of an array is 8 (it is 16 on 64-bit). This happens to be the same size as a pointer and a double. I see cases where we start with (loc5 - 1) * 8 + 8 which morph transforms to loc5 * 8 - 8 + 8 => loc5 * 8, thus we no longer have an "offset" into the array data (and at runtime loc5 better be > 0, which is checked by the bounds check).

The result is that we give the ARR_ADDR a new, unique VN and we lose some CSEs.

[BruceForstall]

GitHub is confused; all the tests passed.

Diffsas described

[BruceForstall]

If the ARR_ADDR node doesn't make sense, namely if the array offset does not appear to be in the array, then bail.

[BruceForstall]

Extracted from #94250

[BruceForstall]

There are a few diffs in ILGEN tests that construct array index expressions with negative indices.

[BruceForstall]

With JitOptRepeat we end up with the following situation.

We hit the assert:

Assertion failed '(constIndexOffset % elemSize) == 0'

(e.g.,

System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1+StateMachineBox`1[int,System.IO.Stream+<ReadAtLeastAsyncCore>d__46]:System.Threading.Tasks.Sources.IValueTaskSource.GetResult(short):this'

We start with:

[000895] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000894] ----------- \--* ADD byref
[000885] ----------- +--* LCL_VAR ref V56 tmp54
[000893] ----------- \--* ADD long
[000891] ----------- +--* MUL long
[000889] ---------U- | +--* CAST long <- uint
[000886] ----------- | | \--* LCL_VAR int V14 tmp12
[000890] -------N--- | \--* CNS_INT long 128
[000892] ----------- \--* CNS_INT long 16

After optimization, this becomes:

N018 ( 2, 2) [000895] -A---O----- \--* ARR_ADDR byref Internal.PaddedReference[] $641
N017 ( 2, 2) [000894] -A-----N--- \--* ADD byref $199
N007 ( 1, 1) [000885] ----------- +--* LCL_VAR ref V56 tmp54 u:1 (last use) $84
N016 ( 7, 10) [000995] -A--------- \--* COMMA long $687
N014 ( 6, 9) [000993] DA--------- +--* STORE_LCL_VAR long V62 cse3 d:1 $VN.Void
N013 ( 6, 9) [000893] ----------- | \--* ADD long $687
N011 ( 4, 6) [000891] ----------- | +--* LSH long $686
N009 ( 2, 3) [000889] ---------U- | | +--* CAST long <- uint $685
N008 ( 1, 1) [000886] ----------- | | | \--* LCL_VAR int V14 tmp12 u:1 $1f5
N010 ( 1, 2) [000890] -------N--- | | \--* CNS_INT long 7 $14e
N012 ( 1, 2) [000892] ----------- | \--* CNS_INT long 16 $146
N015 ( 1, 1) [000994] ----------- \--* LCL_VAR long V62 cse3 u:1 $687

On the next iteration of JitOptRepeat, the CSE injected COMMA is not parseable by GenTreeArrAddr::ParseArrayAddress. We could add tunnel through for this CSE def.

However, it might be harder for the CSE use. In that case, we start with something similar:

[000941] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000940] ----------- \--* ADD byref
[000931] ----------- +--* LCL_VAR ref V57 tmp55
[000939] ----------- \--* ADD long
[000937] ----------- +--* MUL long
[000935] ---------U- | +--* CAST long <- uint
[000932] ----------- | | \--* LCL_VAR int V14 tmp12 (last use)
[000936] -------N--- | \--* CNS_INT long 128
[000938] ----------- \--* CNS_INT long 16

and, after CSE, have:

N006 ( 2, 2) [000941] -----O-N--- \--* ARR_ADDR byref Internal.PaddedReference[]
N005 ( 2, 2) [000940] -------N--- \--* ADD byref
N003 ( 1, 1) [000931] ----------- +--* LCL_VAR ref V57 tmp55 u:1 (last use)
N004 ( 1, 1) [000996] ----------- \--* LCL_VAR long V62 cse3 u:1 (last use)

This would require parsing the CSE value number for [000996], not the tree.

Since this is VN iteration #2, I'm not sure we lose anything by assigning the ARR_ADDR a new, unique value number when we fail to parse the tree: we've already optimized it once.

[BruceForstall]

@jakobbotsch PTAL
cc @dotnet/jit-contrib

[BruceForstall]

It's interesting that there are diffs on x86, apparently because the offset of data of an array from the base of an array is 8 (it is 16 on 64-bit). This happens to be the same size as a pointer and a double. I see cases where we start with (loc5 - 1) * 8 + 8 which morph transforms to loc5 * 8 - 8 + 8 => loc5 * 8, thus we no longer have an "offset" into the array data (and at runtime loc5 better be > 0, which is checked by the bounds check).

The result is that we give the ARR_ADDR a new, unique VN and we lose some CSEs.

[BruceForstall]

GitHub is confused; all the tests passed.

Diffsas described

[BruceForstall]

If the ARR_ADDR node doesn't make sense, namely if the array offset does not appear to be in the array, then bail.

[BruceForstall]

Extracted from #94250

[BruceForstall]

There are a few diffs in ILGEN tests that construct array index expressions with negative indices.

[BruceForstall]

With JitOptRepeat we end up with the following situation.

We hit the assert:

Assertion failed '(constIndexOffset % elemSize) == 0'

(e.g.,

System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1+StateMachineBox`1[int,System.IO.Stream+<ReadAtLeastAsyncCore>d__46]:System.Threading.Tasks.Sources.IValueTaskSource.GetResult(short):this'

We start with:

[000895] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000894] ----------- \--* ADD byref
[000885] ----------- +--* LCL_VAR ref V56 tmp54
[000893] ----------- \--* ADD long
[000891] ----------- +--* MUL long
[000889] ---------U- | +--* CAST long <- uint
[000886] ----------- | | \--* LCL_VAR int V14 tmp12
[000890] -------N--- | \--* CNS_INT long 128
[000892] ----------- \--* CNS_INT long 16

After optimization, this becomes:

N018 ( 2, 2) [000895] -A---O----- \--* ARR_ADDR byref Internal.PaddedReference[] $641
N017 ( 2, 2) [000894] -A-----N--- \--* ADD byref $199
N007 ( 1, 1) [000885] ----------- +--* LCL_VAR ref V56 tmp54 u:1 (last use) $84
N016 ( 7, 10) [000995] -A--------- \--* COMMA long $687
N014 ( 6, 9) [000993] DA--------- +--* STORE_LCL_VAR long V62 cse3 d:1 $VN.Void
N013 ( 6, 9) [000893] ----------- | \--* ADD long $687
N011 ( 4, 6) [000891] ----------- | +--* LSH long $686
N009 ( 2, 3) [000889] ---------U- | | +--* CAST long <- uint $685
N008 ( 1, 1) [000886] ----------- | | | \--* LCL_VAR int V14 tmp12 u:1 $1f5
N010 ( 1, 2) [000890] -------N--- | | \--* CNS_INT long 7 $14e
N012 ( 1, 2) [000892] ----------- | \--* CNS_INT long 16 $146
N015 ( 1, 1) [000994] ----------- \--* LCL_VAR long V62 cse3 u:1 $687

On the next iteration of JitOptRepeat, the CSE injected COMMA is not parseable by GenTreeArrAddr::ParseArrayAddress. We could add tunnel through for this CSE def.

However, it might be harder for the CSE use. In that case, we start with something similar:

[000941] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000940] ----------- \--* ADD byref
[000931] ----------- +--* LCL_VAR ref V57 tmp55
[000939] ----------- \--* ADD long
[000937] ----------- +--* MUL long
[000935] ---------U- | +--* CAST long <- uint
[000932] ----------- | | \--* LCL_VAR int V14 tmp12 (last use)
[000936] -------N--- | \--* CNS_INT long 128
[000938] ----------- \--* CNS_INT long 16

and, after CSE, have:

N006 ( 2, 2) [000941] -----O-N--- \--* ARR_ADDR byref Internal.PaddedReference[]
N005 ( 2, 2) [000940] -------N--- \--* ADD byref
N003 ( 1, 1) [000931] ----------- +--* LCL_VAR ref V57 tmp55 u:1 (last use)
N004 ( 1, 1) [000996] ----------- \--* LCL_VAR long V62 cse3 u:1 (last use)

This would require parsing the CSE value number for [000996], not the tree.

Since this is VN iteration #2, I'm not sure we lose anything by assigning the ARR_ADDR a new, unique value number when we fail to parse the tree: we've already optimized it once.

[BruceForstall]

@jakobbotsch PTAL
cc @dotnet/jit-contrib

[BruceForstall]

It's interesting that there are diffs on x86, apparently because the offset of data of an array from the base of an array is 8 (it is 16 on 64-bit). This happens to be the same size as a pointer and a double. I see cases where we start with (loc5 - 1) * 8 + 8 which morph transforms to loc5 * 8 - 8 + 8 => loc5 * 8, thus we no longer have an "offset" into the array data (and at runtime loc5 better be > 0, which is checked by the bounds check).

The result is that we give the ARR_ADDR a new, unique VN and we lose some CSEs.

[BruceForstall]

GitHub is confused; all the tests passed.

Diffsas described

[BruceForstall]

If the ARR_ADDR node doesn't make sense, namely if the array offset does not appear to be in the array, then bail.

[BruceForstall]

Extracted from #94250

[BruceForstall]

There are a few diffs in ILGEN tests that construct array index expressions with negative indices.

[BruceForstall]

With JitOptRepeat we end up with the following situation.

We hit the assert:

Assertion failed '(constIndexOffset % elemSize) == 0'

(e.g.,

System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1+StateMachineBox`1[int,System.IO.Stream+<ReadAtLeastAsyncCore>d__46]:System.Threading.Tasks.Sources.IValueTaskSource.GetResult(short):this'

We start with:

[000895] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000894] ----------- \--* ADD byref
[000885] ----------- +--* LCL_VAR ref V56 tmp54
[000893] ----------- \--* ADD long
[000891] ----------- +--* MUL long
[000889] ---------U- | +--* CAST long <- uint
[000886] ----------- | | \--* LCL_VAR int V14 tmp12
[000890] -------N--- | \--* CNS_INT long 128
[000892] ----------- \--* CNS_INT long 16

After optimization, this becomes:

N018 ( 2, 2) [000895] -A---O----- \--* ARR_ADDR byref Internal.PaddedReference[] $641
N017 ( 2, 2) [000894] -A-----N--- \--* ADD byref $199
N007 ( 1, 1) [000885] ----------- +--* LCL_VAR ref V56 tmp54 u:1 (last use) $84
N016 ( 7, 10) [000995] -A--------- \--* COMMA long $687
N014 ( 6, 9) [000993] DA--------- +--* STORE_LCL_VAR long V62 cse3 d:1 $VN.Void
N013 ( 6, 9) [000893] ----------- | \--* ADD long $687
N011 ( 4, 6) [000891] ----------- | +--* LSH long $686
N009 ( 2, 3) [000889] ---------U- | | +--* CAST long <- uint $685
N008 ( 1, 1) [000886] ----------- | | | \--* LCL_VAR int V14 tmp12 u:1 $1f5
N010 ( 1, 2) [000890] -------N--- | | \--* CNS_INT long 7 $14e
N012 ( 1, 2) [000892] ----------- | \--* CNS_INT long 16 $146
N015 ( 1, 1) [000994] ----------- \--* LCL_VAR long V62 cse3 u:1 $687

On the next iteration of JitOptRepeat, the CSE injected COMMA is not parseable by GenTreeArrAddr::ParseArrayAddress. We could add tunnel through for this CSE def.

However, it might be harder for the CSE use. In that case, we start with something similar:

[000941] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000940] ----------- \--* ADD byref
[000931] ----------- +--* LCL_VAR ref V57 tmp55
[000939] ----------- \--* ADD long
[000937] ----------- +--* MUL long
[000935] ---------U- | +--* CAST long <- uint
[000932] ----------- | | \--* LCL_VAR int V14 tmp12 (last use)
[000936] -------N--- | \--* CNS_INT long 128
[000938] ----------- \--* CNS_INT long 16

and, after CSE, have:

N006 ( 2, 2) [000941] -----O-N--- \--* ARR_ADDR byref Internal.PaddedReference[]
N005 ( 2, 2) [000940] -------N--- \--* ADD byref
N003 ( 1, 1) [000931] ----------- +--* LCL_VAR ref V57 tmp55 u:1 (last use)
N004 ( 1, 1) [000996] ----------- \--* LCL_VAR long V62 cse3 u:1 (last use)

This would require parsing the CSE value number for [000996], not the tree.

Since this is VN iteration #2, I'm not sure we lose anything by assigning the ARR_ADDR a new, unique value number when we fail to parse the tree: we've already optimized it once.

[BruceForstall]

@jakobbotsch PTAL
cc @dotnet/jit-contrib

[BruceForstall]

It's interesting that there are diffs on x86, apparently because the offset of data of an array from the base of an array is 8 (it is 16 on 64-bit). This happens to be the same size as a pointer and a double. I see cases where we start with (loc5 - 1) * 8 + 8 which morph transforms to loc5 * 8 - 8 + 8 => loc5 * 8, thus we no longer have an "offset" into the array data (and at runtime loc5 better be > 0, which is checked by the bounds check).

The result is that we give the ARR_ADDR a new, unique VN and we lose some CSEs.

[BruceForstall]

GitHub is confused; all the tests passed.

Diffsas described

[BruceForstall]

If the ARR_ADDR node doesn't make sense, namely if the array offset does not appear to be in the array, then bail.

[BruceForstall]

Extracted from #94250

[BruceForstall]

There are a few diffs in ILGEN tests that construct array index expressions with negative indices.

[BruceForstall]

With JitOptRepeat we end up with the following situation.

We hit the assert:

Assertion failed '(constIndexOffset % elemSize) == 0'

(e.g.,

System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1+StateMachineBox`1[int,System.IO.Stream+<ReadAtLeastAsyncCore>d__46]:System.Threading.Tasks.Sources.IValueTaskSource.GetResult(short):this'

We start with:

[000895] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000894] ----------- \--* ADD byref
[000885] ----------- +--* LCL_VAR ref V56 tmp54
[000893] ----------- \--* ADD long
[000891] ----------- +--* MUL long
[000889] ---------U- | +--* CAST long <- uint
[000886] ----------- | | \--* LCL_VAR int V14 tmp12
[000890] -------N--- | \--* CNS_INT long 128
[000892] ----------- \--* CNS_INT long 16

After optimization, this becomes:

N018 ( 2, 2) [000895] -A---O----- \--* ARR_ADDR byref Internal.PaddedReference[] $641
N017 ( 2, 2) [000894] -A-----N--- \--* ADD byref $199
N007 ( 1, 1) [000885] ----------- +--* LCL_VAR ref V56 tmp54 u:1 (last use) $84
N016 ( 7, 10) [000995] -A--------- \--* COMMA long $687
N014 ( 6, 9) [000993] DA--------- +--* STORE_LCL_VAR long V62 cse3 d:1 $VN.Void
N013 ( 6, 9) [000893] ----------- | \--* ADD long $687
N011 ( 4, 6) [000891] ----------- | +--* LSH long $686
N009 ( 2, 3) [000889] ---------U- | | +--* CAST long <- uint $685
N008 ( 1, 1) [000886] ----------- | | | \--* LCL_VAR int V14 tmp12 u:1 $1f5
N010 ( 1, 2) [000890] -------N--- | | \--* CNS_INT long 7 $14e
N012 ( 1, 2) [000892] ----------- | \--* CNS_INT long 16 $146
N015 ( 1, 1) [000994] ----------- \--* LCL_VAR long V62 cse3 u:1 $687

On the next iteration of JitOptRepeat, the CSE injected COMMA is not parseable by GenTreeArrAddr::ParseArrayAddress. We could add tunnel through for this CSE def.

However, it might be harder for the CSE use. In that case, we start with something similar:

[000941] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000940] ----------- \--* ADD byref
[000931] ----------- +--* LCL_VAR ref V57 tmp55
[000939] ----------- \--* ADD long
[000937] ----------- +--* MUL long
[000935] ---------U- | +--* CAST long <- uint
[000932] ----------- | | \--* LCL_VAR int V14 tmp12 (last use)
[000936] -------N--- | \--* CNS_INT long 128
[000938] ----------- \--* CNS_INT long 16

and, after CSE, have:

N006 ( 2, 2) [000941] -----O-N--- \--* ARR_ADDR byref Internal.PaddedReference[]
N005 ( 2, 2) [000940] -------N--- \--* ADD byref
N003 ( 1, 1) [000931] ----------- +--* LCL_VAR ref V57 tmp55 u:1 (last use)
N004 ( 1, 1) [000996] ----------- \--* LCL_VAR long V62 cse3 u:1 (last use)

This would require parsing the CSE value number for [000996], not the tree.

Since this is VN iteration #2, I'm not sure we lose anything by assigning the ARR_ADDR a new, unique value number when we fail to parse the tree: we've already optimized it once.

[BruceForstall]

@jakobbotsch PTAL
cc @dotnet/jit-contrib

[BruceForstall]

It's interesting that there are diffs on x86, apparently because the offset of data of an array from the base of an array is 8 (it is 16 on 64-bit). This happens to be the same size as a pointer and a double. I see cases where we start with (loc5 - 1) * 8 + 8 which morph transforms to loc5 * 8 - 8 + 8 => loc5 * 8, thus we no longer have an "offset" into the array data (and at runtime loc5 better be > 0, which is checked by the bounds check).

The result is that we give the ARR_ADDR a new, unique VN and we lose some CSEs.

[BruceForstall]

GitHub is confused; all the tests passed.

Diffsas described

[BruceForstall]

If the ARR_ADDR node doesn't make sense, namely if the array offset does not appear to be in the array, then bail.

[BruceForstall]

Extracted from #94250

[BruceForstall]

There are a few diffs in ILGEN tests that construct array index expressions with negative indices.

[BruceForstall]

With JitOptRepeat we end up with the following situation.

We hit the assert:

Assertion failed '(constIndexOffset % elemSize) == 0'

(e.g.,

System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder`1+StateMachineBox`1[int,System.IO.Stream+<ReadAtLeastAsyncCore>d__46]:System.Threading.Tasks.Sources.IValueTaskSource.GetResult(short):this'

We start with:

[000895] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000894] ----------- \--* ADD byref
[000885] ----------- +--* LCL_VAR ref V56 tmp54
[000893] ----------- \--* ADD long
[000891] ----------- +--* MUL long
[000889] ---------U- | +--* CAST long <- uint
[000886] ----------- | | \--* LCL_VAR int V14 tmp12
[000890] -------N--- | \--* CNS_INT long 128
[000892] ----------- \--* CNS_INT long 16

After optimization, this becomes:

N018 ( 2, 2) [000895] -A---O----- \--* ARR_ADDR byref Internal.PaddedReference[] $641
N017 ( 2, 2) [000894] -A-----N--- \--* ADD byref $199
N007 ( 1, 1) [000885] ----------- +--* LCL_VAR ref V56 tmp54 u:1 (last use) $84
N016 ( 7, 10) [000995] -A--------- \--* COMMA long $687
N014 ( 6, 9) [000993] DA--------- +--* STORE_LCL_VAR long V62 cse3 d:1 $VN.Void
N013 ( 6, 9) [000893] ----------- | \--* ADD long $687
N011 ( 4, 6) [000891] ----------- | +--* LSH long $686
N009 ( 2, 3) [000889] ---------U- | | +--* CAST long <- uint $685
N008 ( 1, 1) [000886] ----------- | | | \--* LCL_VAR int V14 tmp12 u:1 $1f5
N010 ( 1, 2) [000890] -------N--- | | \--* CNS_INT long 7 $14e
N012 ( 1, 2) [000892] ----------- | \--* CNS_INT long 16 $146
N015 ( 1, 1) [000994] ----------- \--* LCL_VAR long V62 cse3 u:1 $687

On the next iteration of JitOptRepeat, the CSE injected COMMA is not parseable by GenTreeArrAddr::ParseArrayAddress. We could add tunnel through for this CSE def.

However, it might be harder for the CSE use. In that case, we start with something similar:

[000941] -----O----- \--* ARR_ADDR byref Internal.PaddedReference[]
[000940] ----------- \--* ADD byref
[000931] ----------- +--* LCL_VAR ref V57 tmp55
[000939] ----------- \--* ADD long
[000937] ----------- +--* MUL long
[000935] ---------U- | +--* CAST long <- uint
[000932] ----------- | | \--* LCL_VAR int V14 tmp12 (last use)
[000936] -------N--- | \--* CNS_INT long 128
[000938] ----------- \--* CNS_INT long 16

and, after CSE, have:

N006 ( 2, 2) [000941] -----O-N--- \--* ARR_ADDR byref Internal.PaddedReference[]
N005 ( 2, 2) [000940] -------N--- \--* ADD byref
N003 ( 1, 1) [000931] ----------- +--* LCL_VAR ref V57 tmp55 u:1 (last use)
N004 ( 1, 1) [000996] ----------- \--* LCL_VAR long V62 cse3 u:1 (last use)

This would require parsing the CSE value number for [000996], not the tree.

Since this is VN iteration #2, I'm not sure we lose anything by assigning the ARR_ADDR a new, unique value number when we fail to parse the tree: we've already optimized it once.

[BruceForstall]

@jakobbotsch PTAL
cc @dotnet/jit-contrib

[BruceForstall]

It's interesting that there are diffs on x86, apparently because the offset of data of an array from the base of an array is 8 (it is 16 on 64-bit). This happens to be the same size as a pointer and a double. I see cases where we start with (loc5 - 1) * 8 + 8 which morph transforms to loc5 * 8 - 8 + 8 => loc5 * 8, thus we no longer have an "offset" into the array data (and at runtime loc5 better be > 0, which is checked by the bounds check).

The result is that we give the ARR_ADDR a new, unique VN and we lose some CSEs.

[BruceForstall]

GitHub is confused; all the tests passed.

Diffsas described