Check for non-valid compare chains (#76566)

[a74nh]

Small change to not fall over when finding contained nodes that aren't a valid compare chain.

[ghost]

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

Issue Details

---

null

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

[jakobbotsch]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

[a74nh]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

Agreed it's a horrible name. But, when isolated, the function isn't really connected to compare chains, which is why it ended up as it did.

I've switched it to isContainedCompareChainSegment(), and tidied up IsValidCompareChain() a little.

[a74nh]

CI build errors all seem to be network issues

[jakobbotsch]

Will rerun CI.

[a74nh]

I ran spmidiff across the coreclr testsuite. There was a single function that differed with this patch.

Before:

 ldr w0, [x19, #0x5C] movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
and w0, w1, w0, ASR #5
cbz w0, G_M4252_IG05

With patch:

 ldr w0, [x19, #0x5C]
asr w0, w0, #5
movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
tst w1, w0
beq G_M4252_IG05

IR:

N011 ( 15, 28) [000650] ----GO----- * JTRUE void $301
N010 ( 13, 26) [000649] J---GO-N--- \--* EQ int <l:$2b7, c:$2b8>
N008 ( 11, 23) [000817] ----GO----- +--* AND int <l:$2b3, c:$2b4>
N006 ( 6, 6) [000815] ----GO----- | +--* RSH int <l:$2af, c:$2b0>
N004 ( 4, 3) [000813] n---GO----- | | +--* IND int <l:$2ab, c:$2ac>
N003 ( 3, 4) [002556] -------N--- | | | \--* ADD byref $180
N001 ( 1, 1) [000643] ----------- | | | +--* LCL_VAR ref V00 this u:1 $80
N002 ( 1, 2) [002555] ----------- | | | \--* CNS_INT long 92 Fseq[hackishFieldName] $140
N005 ( 1, 2) [000814] ----------- | | \--* CNS_INT int 5 $45
N007 ( 4, 16) [000816] ----------- | \--* CNS_INT int 0x310FB $4a
N009 ( 1, 2) [000648] ----------- \--* CNS_INT int 0 $40

Code is fine in both. But slightly longer with my patch.

Change is due to the check in Lowering::OptimizeConstCompare(). The code says if this is a Contained Compare Chain Segment then don't do the OptimizeConstCompare optimisations.

With my patch, it correctly determines 817 isn't a Compare Chain and then proceeds to optimise away the AND with constant and convert the JTRUE into a JCMP.

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

[jakobbotsch]

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

I would keep the code as is, I think this tiny regression is not something to worry about since we were just getting it right by chance. If we want to fix this, we should make the make the AND optimization aware of this interaction with the other optimization.

[jakobbotsch]

Thanks!

[a74nh]

Small change to not fall over when finding contained nodes that aren't a valid compare chain.

[ghost]

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

Issue Details

---

null

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

[jakobbotsch]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

[a74nh]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

Agreed it's a horrible name. But, when isolated, the function isn't really connected to compare chains, which is why it ended up as it did.

I've switched it to isContainedCompareChainSegment(), and tidied up IsValidCompareChain() a little.

[a74nh]

CI build errors all seem to be network issues

[jakobbotsch]

Will rerun CI.

[a74nh]

I ran spmidiff across the coreclr testsuite. There was a single function that differed with this patch.

Before:

 ldr w0, [x19, #0x5C] movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
and w0, w1, w0, ASR #5
cbz w0, G_M4252_IG05

With patch:

 ldr w0, [x19, #0x5C]
asr w0, w0, #5
movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
tst w1, w0
beq G_M4252_IG05

IR:

N011 ( 15, 28) [000650] ----GO----- * JTRUE void $301
N010 ( 13, 26) [000649] J---GO-N--- \--* EQ int <l:$2b7, c:$2b8>
N008 ( 11, 23) [000817] ----GO----- +--* AND int <l:$2b3, c:$2b4>
N006 ( 6, 6) [000815] ----GO----- | +--* RSH int <l:$2af, c:$2b0>
N004 ( 4, 3) [000813] n---GO----- | | +--* IND int <l:$2ab, c:$2ac>
N003 ( 3, 4) [002556] -------N--- | | | \--* ADD byref $180
N001 ( 1, 1) [000643] ----------- | | | +--* LCL_VAR ref V00 this u:1 $80
N002 ( 1, 2) [002555] ----------- | | | \--* CNS_INT long 92 Fseq[hackishFieldName] $140
N005 ( 1, 2) [000814] ----------- | | \--* CNS_INT int 5 $45
N007 ( 4, 16) [000816] ----------- | \--* CNS_INT int 0x310FB $4a
N009 ( 1, 2) [000648] ----------- \--* CNS_INT int 0 $40

Code is fine in both. But slightly longer with my patch.

Change is due to the check in Lowering::OptimizeConstCompare(). The code says if this is a Contained Compare Chain Segment then don't do the OptimizeConstCompare optimisations.

With my patch, it correctly determines 817 isn't a Compare Chain and then proceeds to optimise away the AND with constant and convert the JTRUE into a JCMP.

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

[jakobbotsch]

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

I would keep the code as is, I think this tiny regression is not something to worry about since we were just getting it right by chance. If we want to fix this, we should make the make the AND optimization aware of this interaction with the other optimization.

[jakobbotsch]

Thanks!

[a74nh]

Small change to not fall over when finding contained nodes that aren't a valid compare chain.

[ghost]

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

Issue Details

---

null

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

[jakobbotsch]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

[a74nh]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

Agreed it's a horrible name. But, when isolated, the function isn't really connected to compare chains, which is why it ended up as it did.

I've switched it to isContainedCompareChainSegment(), and tidied up IsValidCompareChain() a little.

[a74nh]

CI build errors all seem to be network issues

[jakobbotsch]

Will rerun CI.

[a74nh]

I ran spmidiff across the coreclr testsuite. There was a single function that differed with this patch.

Before:

 ldr w0, [x19, #0x5C] movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
and w0, w1, w0, ASR #5
cbz w0, G_M4252_IG05

With patch:

 ldr w0, [x19, #0x5C]
asr w0, w0, #5
movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
tst w1, w0
beq G_M4252_IG05

IR:

N011 ( 15, 28) [000650] ----GO----- * JTRUE void $301
N010 ( 13, 26) [000649] J---GO-N--- \--* EQ int <l:$2b7, c:$2b8>
N008 ( 11, 23) [000817] ----GO----- +--* AND int <l:$2b3, c:$2b4>
N006 ( 6, 6) [000815] ----GO----- | +--* RSH int <l:$2af, c:$2b0>
N004 ( 4, 3) [000813] n---GO----- | | +--* IND int <l:$2ab, c:$2ac>
N003 ( 3, 4) [002556] -------N--- | | | \--* ADD byref $180
N001 ( 1, 1) [000643] ----------- | | | +--* LCL_VAR ref V00 this u:1 $80
N002 ( 1, 2) [002555] ----------- | | | \--* CNS_INT long 92 Fseq[hackishFieldName] $140
N005 ( 1, 2) [000814] ----------- | | \--* CNS_INT int 5 $45
N007 ( 4, 16) [000816] ----------- | \--* CNS_INT int 0x310FB $4a
N009 ( 1, 2) [000648] ----------- \--* CNS_INT int 0 $40

Code is fine in both. But slightly longer with my patch.

Change is due to the check in Lowering::OptimizeConstCompare(). The code says if this is a Contained Compare Chain Segment then don't do the OptimizeConstCompare optimisations.

With my patch, it correctly determines 817 isn't a Compare Chain and then proceeds to optimise away the AND with constant and convert the JTRUE into a JCMP.

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

[jakobbotsch]

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

I would keep the code as is, I think this tiny regression is not something to worry about since we were just getting it right by chance. If we want to fix this, we should make the make the AND optimization aware of this interaction with the other optimization.

[jakobbotsch]

Thanks!

[a74nh]

Small change to not fall over when finding contained nodes that aren't a valid compare chain.

[ghost]

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

Issue Details

---

null

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

[jakobbotsch]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

[a74nh]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

Agreed it's a horrible name. But, when isolated, the function isn't really connected to compare chains, which is why it ended up as it did.

I've switched it to isContainedCompareChainSegment(), and tidied up IsValidCompareChain() a little.

[a74nh]

CI build errors all seem to be network issues

[jakobbotsch]

Will rerun CI.

[a74nh]

I ran spmidiff across the coreclr testsuite. There was a single function that differed with this patch.

Before:

 ldr w0, [x19, #0x5C] movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
and w0, w1, w0, ASR #5
cbz w0, G_M4252_IG05

With patch:

 ldr w0, [x19, #0x5C]
asr w0, w0, #5
movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
tst w1, w0
beq G_M4252_IG05

IR:

N011 ( 15, 28) [000650] ----GO----- * JTRUE void $301
N010 ( 13, 26) [000649] J---GO-N--- \--* EQ int <l:$2b7, c:$2b8>
N008 ( 11, 23) [000817] ----GO----- +--* AND int <l:$2b3, c:$2b4>
N006 ( 6, 6) [000815] ----GO----- | +--* RSH int <l:$2af, c:$2b0>
N004 ( 4, 3) [000813] n---GO----- | | +--* IND int <l:$2ab, c:$2ac>
N003 ( 3, 4) [002556] -------N--- | | | \--* ADD byref $180
N001 ( 1, 1) [000643] ----------- | | | +--* LCL_VAR ref V00 this u:1 $80
N002 ( 1, 2) [002555] ----------- | | | \--* CNS_INT long 92 Fseq[hackishFieldName] $140
N005 ( 1, 2) [000814] ----------- | | \--* CNS_INT int 5 $45
N007 ( 4, 16) [000816] ----------- | \--* CNS_INT int 0x310FB $4a
N009 ( 1, 2) [000648] ----------- \--* CNS_INT int 0 $40

Code is fine in both. But slightly longer with my patch.

Change is due to the check in Lowering::OptimizeConstCompare(). The code says if this is a Contained Compare Chain Segment then don't do the OptimizeConstCompare optimisations.

With my patch, it correctly determines 817 isn't a Compare Chain and then proceeds to optimise away the AND with constant and convert the JTRUE into a JCMP.

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

[jakobbotsch]

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

I would keep the code as is, I think this tiny regression is not something to worry about since we were just getting it right by chance. If we want to fix this, we should make the make the AND optimization aware of this interaction with the other optimization.

[jakobbotsch]

Thanks!

[a74nh]

Small change to not fall over when finding contained nodes that aren't a valid compare chain.

[ghost]

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

Issue Details

---

null

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

[jakobbotsch]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

[a74nh]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

Agreed it's a horrible name. But, when isolated, the function isn't really connected to compare chains, which is why it ended up as it did.

I've switched it to isContainedCompareChainSegment(), and tidied up IsValidCompareChain() a little.

[a74nh]

CI build errors all seem to be network issues

[jakobbotsch]

Will rerun CI.

[a74nh]

I ran spmidiff across the coreclr testsuite. There was a single function that differed with this patch.

Before:

 ldr w0, [x19, #0x5C] movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
and w0, w1, w0, ASR #5
cbz w0, G_M4252_IG05

With patch:

 ldr w0, [x19, #0x5C]
asr w0, w0, #5
movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
tst w1, w0
beq G_M4252_IG05

IR:

N011 ( 15, 28) [000650] ----GO----- * JTRUE void $301
N010 ( 13, 26) [000649] J---GO-N--- \--* EQ int <l:$2b7, c:$2b8>
N008 ( 11, 23) [000817] ----GO----- +--* AND int <l:$2b3, c:$2b4>
N006 ( 6, 6) [000815] ----GO----- | +--* RSH int <l:$2af, c:$2b0>
N004 ( 4, 3) [000813] n---GO----- | | +--* IND int <l:$2ab, c:$2ac>
N003 ( 3, 4) [002556] -------N--- | | | \--* ADD byref $180
N001 ( 1, 1) [000643] ----------- | | | +--* LCL_VAR ref V00 this u:1 $80
N002 ( 1, 2) [002555] ----------- | | | \--* CNS_INT long 92 Fseq[hackishFieldName] $140
N005 ( 1, 2) [000814] ----------- | | \--* CNS_INT int 5 $45
N007 ( 4, 16) [000816] ----------- | \--* CNS_INT int 0x310FB $4a
N009 ( 1, 2) [000648] ----------- \--* CNS_INT int 0 $40

Code is fine in both. But slightly longer with my patch.

Change is due to the check in Lowering::OptimizeConstCompare(). The code says if this is a Contained Compare Chain Segment then don't do the OptimizeConstCompare optimisations.

With my patch, it correctly determines 817 isn't a Compare Chain and then proceeds to optimise away the AND with constant and convert the JTRUE into a JCMP.

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

[jakobbotsch]

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

I would keep the code as is, I think this tiny regression is not something to worry about since we were just getting it right by chance. If we want to fix this, we should make the make the AND optimization aware of this interaction with the other optimization.

[jakobbotsch]

Thanks!

[a74nh]

Small change to not fall over when finding contained nodes that aren't a valid compare chain.

[ghost]

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

Issue Details

---

null

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

[jakobbotsch]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

[a74nh]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

Agreed it's a horrible name. But, when isolated, the function isn't really connected to compare chains, which is why it ended up as it did.

I've switched it to isContainedCompareChainSegment(), and tidied up IsValidCompareChain() a little.

[a74nh]

CI build errors all seem to be network issues

[jakobbotsch]

Will rerun CI.

[a74nh]

I ran spmidiff across the coreclr testsuite. There was a single function that differed with this patch.

Before:

 ldr w0, [x19, #0x5C] movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
and w0, w1, w0, ASR #5
cbz w0, G_M4252_IG05

With patch:

 ldr w0, [x19, #0x5C]
asr w0, w0, #5
movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
tst w1, w0
beq G_M4252_IG05

IR:

N011 ( 15, 28) [000650] ----GO----- * JTRUE void $301
N010 ( 13, 26) [000649] J---GO-N--- \--* EQ int <l:$2b7, c:$2b8>
N008 ( 11, 23) [000817] ----GO----- +--* AND int <l:$2b3, c:$2b4>
N006 ( 6, 6) [000815] ----GO----- | +--* RSH int <l:$2af, c:$2b0>
N004 ( 4, 3) [000813] n---GO----- | | +--* IND int <l:$2ab, c:$2ac>
N003 ( 3, 4) [002556] -------N--- | | | \--* ADD byref $180
N001 ( 1, 1) [000643] ----------- | | | +--* LCL_VAR ref V00 this u:1 $80
N002 ( 1, 2) [002555] ----------- | | | \--* CNS_INT long 92 Fseq[hackishFieldName] $140
N005 ( 1, 2) [000814] ----------- | | \--* CNS_INT int 5 $45
N007 ( 4, 16) [000816] ----------- | \--* CNS_INT int 0x310FB $4a
N009 ( 1, 2) [000648] ----------- \--* CNS_INT int 0 $40

Code is fine in both. But slightly longer with my patch.

Change is due to the check in Lowering::OptimizeConstCompare(). The code says if this is a Contained Compare Chain Segment then don't do the OptimizeConstCompare optimisations.

With my patch, it correctly determines 817 isn't a Compare Chain and then proceeds to optimise away the AND with constant and convert the JTRUE into a JCMP.

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

[jakobbotsch]

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

I would keep the code as is, I think this tiny regression is not something to worry about since we were just getting it right by chance. If we want to fix this, we should make the make the AND optimization aware of this interaction with the other optimization.

[jakobbotsch]

Thanks!

[a74nh]

Small change to not fall over when finding contained nodes that aren't a valid compare chain.

[ghost]

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

Issue Details

---

null

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

[jakobbotsch]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

[a74nh]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

Agreed it's a horrible name. But, when isolated, the function isn't really connected to compare chains, which is why it ended up as it did.

I've switched it to isContainedCompareChainSegment(), and tidied up IsValidCompareChain() a little.

[a74nh]

CI build errors all seem to be network issues

[jakobbotsch]

Will rerun CI.

[a74nh]

I ran spmidiff across the coreclr testsuite. There was a single function that differed with this patch.

Before:

 ldr w0, [x19, #0x5C] movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
and w0, w1, w0, ASR #5
cbz w0, G_M4252_IG05

With patch:

 ldr w0, [x19, #0x5C]
asr w0, w0, #5
movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
tst w1, w0
beq G_M4252_IG05

IR:

N011 ( 15, 28) [000650] ----GO----- * JTRUE void $301
N010 ( 13, 26) [000649] J---GO-N--- \--* EQ int <l:$2b7, c:$2b8>
N008 ( 11, 23) [000817] ----GO----- +--* AND int <l:$2b3, c:$2b4>
N006 ( 6, 6) [000815] ----GO----- | +--* RSH int <l:$2af, c:$2b0>
N004 ( 4, 3) [000813] n---GO----- | | +--* IND int <l:$2ab, c:$2ac>
N003 ( 3, 4) [002556] -------N--- | | | \--* ADD byref $180
N001 ( 1, 1) [000643] ----------- | | | +--* LCL_VAR ref V00 this u:1 $80
N002 ( 1, 2) [002555] ----------- | | | \--* CNS_INT long 92 Fseq[hackishFieldName] $140
N005 ( 1, 2) [000814] ----------- | | \--* CNS_INT int 5 $45
N007 ( 4, 16) [000816] ----------- | \--* CNS_INT int 0x310FB $4a
N009 ( 1, 2) [000648] ----------- \--* CNS_INT int 0 $40

Code is fine in both. But slightly longer with my patch.

Change is due to the check in Lowering::OptimizeConstCompare(). The code says if this is a Contained Compare Chain Segment then don't do the OptimizeConstCompare optimisations.

With my patch, it correctly determines 817 isn't a Compare Chain and then proceeds to optimise away the AND with constant and convert the JTRUE into a JCMP.

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

[jakobbotsch]

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

I would keep the code as is, I think this tiny regression is not something to worry about since we were just getting it right by chance. If we want to fix this, we should make the make the AND optimization aware of this interaction with the other optimization.

[jakobbotsch]

Thanks!

[a74nh]

Small change to not fall over when finding contained nodes that aren't a valid compare chain.

[ghost]

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

Issue Details

---

null

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

[jakobbotsch]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

[a74nh]

Can we rename isContainedAndNotIntOrIImmed() => isContainedPartOfCompareChain() or isPartOfCompareChain and make it check this condition instead?

Agreed it's a horrible name. But, when isolated, the function isn't really connected to compare chains, which is why it ended up as it did.

I've switched it to isContainedCompareChainSegment(), and tidied up IsValidCompareChain() a little.

[a74nh]

CI build errors all seem to be network issues

[jakobbotsch]

Will rerun CI.

[a74nh]

I ran spmidiff across the coreclr testsuite. There was a single function that differed with this patch.

Before:

 ldr w0, [x19, #0x5C] movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
and w0, w1, w0, ASR #5
cbz w0, G_M4252_IG05

With patch:

 ldr w0, [x19, #0x5C]
asr w0, w0, #5
movz w1, #0xD1FFAB1E
movk w1, #3 LSL #16
tst w1, w0
beq G_M4252_IG05

IR:

N011 ( 15, 28) [000650] ----GO----- * JTRUE void $301
N010 ( 13, 26) [000649] J---GO-N--- \--* EQ int <l:$2b7, c:$2b8>
N008 ( 11, 23) [000817] ----GO----- +--* AND int <l:$2b3, c:$2b4>
N006 ( 6, 6) [000815] ----GO----- | +--* RSH int <l:$2af, c:$2b0>
N004 ( 4, 3) [000813] n---GO----- | | +--* IND int <l:$2ab, c:$2ac>
N003 ( 3, 4) [002556] -------N--- | | | \--* ADD byref $180
N001 ( 1, 1) [000643] ----------- | | | +--* LCL_VAR ref V00 this u:1 $80
N002 ( 1, 2) [002555] ----------- | | | \--* CNS_INT long 92 Fseq[hackishFieldName] $140
N005 ( 1, 2) [000814] ----------- | | \--* CNS_INT int 5 $45
N007 ( 4, 16) [000816] ----------- | \--* CNS_INT int 0x310FB $4a
N009 ( 1, 2) [000648] ----------- \--* CNS_INT int 0 $40

Code is fine in both. But slightly longer with my patch.

Change is due to the check in Lowering::OptimizeConstCompare(). The code says if this is a Contained Compare Chain Segment then don't do the OptimizeConstCompare optimisations.

With my patch, it correctly determines 817 isn't a Compare Chain and then proceeds to optimise away the AND with constant and convert the JTRUE into a JCMP.

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

[jakobbotsch]

Without my patch it incorrectly decides 817 is a Compare Chain and returns without removing the AND or converting JTRUE to JCMP. But then later on produces better code via the putting the shift into the AND and CBZ.

I'm thinking if the check in OptimizeConstCompare() should be reverted back to checking for isContainedAndNotIntOrIImmed().

I would keep the code as is, I think this tiny regression is not something to worry about since we were just getting it right by chance. If we want to fix this, we should make the make the AND optimization aware of this interaction with the other optimization.

[jakobbotsch]

Thanks!