Add simple Else conditions to If Conversion #77728

a74nh · 2022-11-01T16:08:28Z

For example:
if (x < 7) { a = 5; } else { a = 9; }
a = (cond) ? b : c;

The else condition must write to the same variable as the then statement.

This patch builds on top of #73472

It refactors a lot of the code in the If Conversion pass. Various parts are split into new functions. Following the style of other passes, I put everything inside it's own OptIfConversionDsc class too.

Detection is slightly reworked - It now finds a valid flow and then checks all the statements inside that flow are valid. I think this is slightly better for performance.

The existing if conversion logic should be unchanged - all the checks and modifications are the same.

This does not catch GT_RETURN cases, for example:
return (a==b) ? 1 : 0
It should be possible to add that on top of this.

For example: if (x < 7) { a = 5; } else { a = 9; } a = (cond) ? b : c; The else condition must write to the same variable as the then statement.

ghost · 2022-11-01T16:08:44Z

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

Issue Details

For example:
if (x < 7) { a = 5; } else { a = 9; }
a = (cond) ? b : c;

The else condition must write to the same variable as the then statement.

This patch builds on top of #73472

It refactors a lot of the code in the If Conversion pass. Various parts are split into new functions. Following the style of other passes, I put everything inside it's own OptIfConversionDsc class too.

Detection is slightly reworked - It now finds a valid flow and then checks all the statements inside that flow are valid. I think this is slightly better for performance.

The existing if conversion logic should be unchanged - all the checks and modifications are the same.

This does not catch GT_RETURN cases, for example:
return (a==b) ? 1 : 0
It should be possible to add that on top of this.

Author:	a74nh
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

a74nh · 2022-11-01T16:11:05Z

Note, due to #77723, after rebasing on top of the latest head, I was no longer able to build anything. All changes are contained within optIfConvert, so I'm not expecting any issues due to the rebase.

a74nh · 2022-11-01T16:23:09Z

The way I've done the SSA feels a little like a fudge, but I wasn't sure how else to do it.

Before optimisation we have two assignments and one jtrue. This gives us two SSA definitions

JTRUE(EQ (A,B))
ASG X = 1   [ssa def 1]
ASG X = 0   [ssa def 2]

After the optimisation, we now just have a single assignment:

ASG X = SELECT(EQ(A,B),1,0)

But, we need two SSA definitions. Especially so given that the next block will probably be a PHI.
To work around this I created a dummy assignment:

ASG X = SELECT(EQ(A,B),1,0)   [ssa def 1]
ASG X = X                     [ssa def 2]

This feels wrong, but the second assignment is eventually optimised away to nothing.

The alternative would be to find all uses of SSA DEF 2 across all blocks and update them to use SSA DEF 1 ? Then somehow remove SSA DEF 2 ?

a74nh · 2022-11-01T16:23:52Z

@kunalspathak, @jakobbotsch

SingleAccretion · 2022-11-01T17:12:01Z

How to handle SSA updates

Since these are currently not possible - move the phase to run after range check and declare it to invalidate SSA?

kunalspathak · 2022-11-02T13:31:31Z

@dotnet/jit-contrib

a74nh · 2022-11-02T14:16:25Z

Since these are currently not possible - move the phase to run after range check and declare it to invalidate SSA?

I tried moving optIfConversion to be after optOptimizeBools and removing all my SSA fix ups (included the ones in HEAD). With that I get no test failures.

Does the SSA state not matter after a certain point?

Is there something specific I should be calling to state that the SSA is invalid?

SingleAccretion · 2022-11-02T14:49:36Z

Does the SSA state not matter after a certain point?

Yes, after the last phase that uses it. Currently this is range check.

Is there something specific I should be calling to state that the SSA is invalid?

No, a comment in compCompile will suffice.

a74nh · 2022-11-02T17:50:50Z

I've been having a quick look at GT_RETURN. It's going to require a little reworking of the code in the PR, but otherwise looks fairly simple to add in. To avoid people re-reviewing another rewrite of the same bits of code, I was thinking it's probably best to get it ready then merge it with this PR.
(Maybe close this PR in the meantime?)

AndyAyersMS · 2022-11-02T18:49:51Z

Does the SSA state not matter after a certain point?

Yes, after the last phase that uses it. Currently this is range check.

Is there something specific I should be calling to state that the SSA is invalid?

No, a comment in compCompile will suffice.

For now you can use assert(!fgDomsComputed) as we explicitly reset this after the SSA phases run.

a74nh · 2022-11-08T09:13:32Z

Some spmidiff results on Arm64 Linux:

[09:09:17] Total bytes of base: 16748340 (overridden on cmd)
[09:09:17] Total bytes of diff: 16741204 (overridden on cmd)
[09:09:17] Total bytes of delta: -7136 (-0.04 % of base)

[09:09:46] Total bytes of base: 154583348 (overridden on cmd)
[09:09:46] Total bytes of diff: 154515596 (overridden on cmd)
[09:09:46] Total bytes of delta: -67752 (-0.04 % of base)

[09:11:41] Total bytes of base: 29368716 (overridden on cmd)
[09:11:41] Total bytes of diff: 29350440 (overridden on cmd)
[09:11:41] Total bytes of delta: -18276 (-0.06 % of base)

Regressions look like just the usual issues around 0s. That should improve once we starting using XZR register for csel.

a74nh · 2022-11-08T09:25:18Z

I'm happy with this patch now. It catches return cases, eg: return (a==b) ? 1 : 0. Also added a lot of DisasmCheck test cases for all supported if conversion cases. Pass has been moved much later, so that all SSA modifications could be removed from the pass.

One test failure, but that looks like just a spurious build failure.

Not happy with the IsPreferredRelop() function, ideally it would use the GenConditionDesc map, but that's in Codegen, and I wasn't sure if was ok be calling into codegen from a front end pass. Essentially, just need to make sure the GenConditionDesc created in genCodeForSelect only requires a single condition (which is now guarded with an assert).

jakobbotsch · 2022-11-08T20:49:19Z

I'm happy with this patch now. It catches return cases, eg: return (a==b) ? 1 : 0. Also added a lot of DisasmCheck test cases for all supported if conversion cases. Pass has been moved much later, so that all SSA modifications could be removed from the pass.

One test failure, but that looks like just a spurious build failure.

Sounds great! I'll try to get to this soon. In the meantime I will run some testing.

Not happy with the IsPreferredRelop() function, ideally it would use the GenConditionDesc map, but that's in Codegen, and I wasn't sure if was ok be calling into codegen from a front end pass. Essentially, just need to make sure the GenConditionDesc created in genCodeForSelect only requires a single condition (which is now guarded with an assert).

I assume this is just heuristically, right? The backend should be able to handle GT_SELECT even in other cases.

jakobbotsch · 2022-11-08T20:49:33Z

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

azure-pipelines · 2022-11-08T20:50:10Z

Azure Pipelines successfully started running 3 pipeline(s).

Change-Id: I61c8419d32d10d005c746cc9ad6ee4e89320d015 CustomizedGitHooks: yes

Change-Id: If61e778cc97e57dd20fd36ed623a1b0870c2b2bd CustomizedGitHooks: yes

a74nh · 2022-11-16T15:06:02Z

It looks like we are missing setting GTF_ORDER_SIDEEFF in some earlier phase.

What's happening here is there is a IND, which much earlier is created from a ARR_ADDR plus BOUNDS_CHECK. At creation of the IND it decides the IND cannot fault (setting GTF_IND_NONFAULTING) - because directly before the IND there is a JTRUE/LE which essentially does the bounds check.

Later during If Conversion, it hoists the IND before the JTRUE/LE, meaning the IND always gets executed.

I don't think we can or would want to recalculate boundary checks during if conversion, so the best I can do with the flags available is:

// Don't believe INDs cannot fault - the if conversion may move it before a check.
if (op2->OperIsIndir() && (op2->gtFlags & GTF_IND_NONFAULTING) != 0)
{
    return false;
}

Incidentally, I think boundary checks always have an else statement, which is why it's only coming up in this patch.

jakobbotsch · 2022-11-16T15:11:45Z

I don't think we can or would want to recalculate boundary checks during if conversion, so the best I can do with the flags available is:

I think the fix should be to make sure we set GTF_ORDER_SIDEEFF on the GT_IND node when we do that optimization. Can you submit a separate PR for it?

a74nh · 2022-11-16T15:15:39Z

I think the fix should be to make sure we set GTF_ORDER_SIDEEFF on the GT_IND node when we do that optimization. Can you submit a separate PR for it?

Yes, I think that'd work. Not sure if a failing test case on main is possible - will give it a go.

a74nh · 2022-11-29T10:33:15Z

Ok, so.... Patch has been rebased after #78698, all review comments should be covered and all the tests are passing.

jakobbotsch · 2022-11-29T10:45:31Z

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

azure-pipelines · 2022-11-29T10:45:54Z

Azure Pipelines successfully started running 2 pipeline(s).

jakobbotsch · 2022-11-29T17:19:32Z

The stress failures so far look like #78912, #78909, #78898

a74nh · 2022-11-29T17:27:35Z

The stress failures so far look like #78912, #78909, #78898

Was just looking at these - then realised they are all X86 failures, so shouldn't be connected to this patch.

jakobbotsch · 2022-11-30T10:53:08Z

I looked at some of the diffs:

8 (13.33 % of base) : 51635.dasm - IfStatements.IfStatements:AndInner(int,int):

-G_M14134_IG02:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
-            tbnz    w0, #0, G_M14134_IG04
-                                               ;; size=4 bbWeight=1    PerfScore 1.00
-G_M14134_IG03:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
-            mov     w2, #5
-            tst     w1, #1
-            csel    w0, w2, w0, eq
-                                               ;; size=12 bbWeight=0.50 PerfScore 0.75
-G_M14134_IG04:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
+G_M14134_IG02:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
+            and     w2, w0, #1
+            and     w3, w1, #1
+            orr     w2, w2, w3
+            mov     w3, #5
+            cmp     w2, #0
+            csel    w0, w3, w0, eq

Let's keep an eye on this one in the benchmarks.
(Side note, not related to this PR: ideally we would have some kind of peephole to get rid of the cmp in favor of orrs here)

8 (3.33 % of base) : 48815.dasm - LinqBenchmarks+<>c:<Where01LinqQueryX>b__7_0(Product):bool:this:

-            ldr     x0, [fp, #0x28]	// [V20 tmp18]
-            ldr     w1, [fp, #0x24]	// [V19 tmp17]
+            ldr     x0, [fp, #0x30]	// [V20 tmp18]
+            ldr     w1, [fp, #0x2C]	// [V19 tmp17]
             orr     x0, x0, x1
-            cbnz    x0, G_M58282_IG04
-            mov     w2, wzr
-            b       G_M58282_IG08
-						;; size=84 bbWeight=0.50 PerfScore 13.50
-G_M58282_IG04:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
-            ldr     w2, [fp, #0x20]	// [V18 tmp16]
-            asr     w0, w2, #31
-            orr     w2, w0, #1
-            b       G_M58282_IG08
+            ldr     w1, [fp, #0x28]	// [V18 tmp16]
+            asr     w1, w1, #31
+            orr     w1, w1, #1
+            cmp     x0, #0
+            csel    w19, w1, wzr, ne
+            b       G_M58282_IG07
+						;; size=96 bbWeight=0.50 PerfScore 15.00

Looks pretty good, the regression comes from different (worse) register allocation.

On the opposite end:

         -24 (-37.50 % of base) : 3407.dasm - System.Numerics.INumber`1[ushort]:Min(ushort,ushort):ushort
         -16 (-36.36 % of base) : 13042.dasm - System.Xml.XmlElement:get_LastNode():System.Xml.XmlLinkedNode:this
         -12 (-33.33 % of base) : 2804.dasm - System.Math:Max(int,int):int
         -12 (-33.33 % of base) : 2806.dasm - System.Math:Max(uint,uint):uint
         -12 (-33.33 % of base) : 2579.dasm - System.Math:Min(int,int):int
         -12 (-33.33 % of base) : 2805.dasm - System.Math:Min(uint,uint):uint

We should keep an eye on benchmarks that may be impacted by these (cc @dotnet/jit-contrib), but overall it seems great to me to use conditional moves for these primitive methods. The diffs look like:
-24 (-37.50 % of base) : 3407.dasm - System.Numerics.INumber1[ushort]:Min(ushort,ushort):ushort`:

 						;; size=8 bbWeight=1    PerfScore 1.50
-G_M1277_IG02:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
+G_M1277_IG02:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
             uxth    w0, w0
             uxth    w1, w1
             cmp     w0, w1
-            beq     G_M1277_IG06
-						;; size=16 bbWeight=1    PerfScore 2.50
-G_M1277_IG03:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
+            csel    w2, w0, w1, lt
             cmp     w0, w1
-            blt     G_M1277_IG05
-            mov     w0, w1
-						;; size=12 bbWeight=0.50 PerfScore 1.00
-G_M1277_IG04:        ; , epilog, nogc, extend
+            csel    w0, w1, w2, eq
+						;; size=24 bbWeight=1    PerfScore 3.00
+G_M1277_IG03:        ; , epilog, nogc, extend
             ldp     fp, lr, [sp], #0x10
             ret     lr
-						;; size=8 bbWeight=0.50 PerfScore 1.00
-G_M1277_IG05:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, epilog, nogc
-            ldp     fp, lr, [sp], #0x10
-            ret     lr
-						;; size=8 bbWeight=0.50 PerfScore 1.00
-G_M1277_IG06:        ; gcVars=0000000000000000 {}, gcrefRegs=0000 {}, byrefRegs=0000 {}, gcvars, byref
-            mov     w0, w1
-						;; size=4 bbWeight=0.50 PerfScore 0.25
-G_M1277_IG07:        ; , epilog, nogc, extend
-            ldp     fp, lr, [sp], #0x10
-            ret     lr
-						;; size=8 bbWeight=0.50 PerfScore 1.00
+						;; size=8 bbWeight=1    PerfScore 2.00

The double compare looks like an instance of #78385.

-12 (-33.33 % of base) : 2804.dasm - System.Math:Max(int,int):int:

-G_M56171_IG01:        ; gcVars=0000000000000000 {}, gcrefRegs=0000 {}, byrefRegs=0000 {}, gcvars, byref, nogc <-- Prolog IG
+G_M56171_IG01:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
             stp     fp, lr, [sp, #-0x10]!
             mov     fp, sp
 						;; size=8 bbWeight=1    PerfScore 1.50
-G_M56171_IG02:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
+G_M56171_IG02:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
             cmp     w0, w1
-            bge     G_M56171_IG05
-						;; size=8 bbWeight=1    PerfScore 1.50
-G_M56171_IG03:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
-            mov     w0, w1
-						;; size=4 bbWeight=0.50 PerfScore 0.25
-G_M56171_IG04:        ; , epilog, nogc, extend
+            csel    w0, w0, w1, ge
+						;; size=8 bbWeight=1    PerfScore 1.00
+G_M56171_IG03:        ; , epilog, nogc, extend
             ldp     fp, lr, [sp], #0x10
             ret     lr
-						;; size=8 bbWeight=0.50 PerfScore 1.00
-G_M56171_IG05:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, epilog, nogc
-            ldp     fp, lr, [sp], #0x10
-            ret     lr
-						;; size=8 bbWeight=0.50 PerfScore 1.00
+						;; size=8 bbWeight=1    PerfScore 2.00

Another one:
-12 (-30.00 % of base) : 3496.dasm - System.Text.ASCIIUtility:FirstCharInUInt32IsAscii(uint):bool:

-G_M6195_IG01:        ; gcVars=0000000000000000 {}, gcrefRegs=0000 {}, byrefRegs=0000 {}, gcvars, byref, nogc <-- Prolog IG
+G_M6195_IG01:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
             stp     fp, lr, [sp, #-0x10]!
             mov     fp, sp
 						;; size=8 bbWeight=1    PerfScore 1.50
-G_M6195_IG02:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
+G_M6195_IG02:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
+            mov     w1, #1
             tst     w0, #0xD1FFAB1E
-            beq     G_M6195_IG05
-						;; size=8 bbWeight=1    PerfScore 1.50
-G_M6195_IG03:        ; gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
-            mov     w0, wzr
-						;; size=4 bbWeight=0.50 PerfScore 0.25
-G_M6195_IG04:        ; , epilog, nogc, extend
+            csel    w0, w1, wzr, eq
+						;; size=12 bbWeight=1    PerfScore 1.50
+G_M6195_IG03:        ; , epilog, nogc, extend
             ldp     fp, lr, [sp], #0x10
             ret     lr
-						;; size=8 bbWeight=0.50 PerfScore 1.00
-G_M6195_IG05:        ; gcVars=0000000000000000 {}, gcrefRegs=0000 {}, byrefRegs=0000 {}, gcvars, byref
-            mov     w0, #1
-						;; size=4 bbWeight=0.50 PerfScore 0.25
-G_M6195_IG06:        ; , epilog, nogc, extend
-            ldp     fp, lr, [sp], #0x10
-            ret     lr
-						;; size=8 bbWeight=0.50 PerfScore 1.00
+						;; size=8 bbWeight=1    PerfScore 2.00

Given the heuristics on if-conversion around loops there's probably not much to worry about here, and overall the diffs look great.

jakobbotsch · 2022-11-30T10:53:52Z

Thanks again!

Add simple Else conditions to If Conversion

d954180

For example: if (x < 7) { a = 5; } else { a = 9; } a = (cond) ? b : c; The else condition must write to the same variable as the then statement.

ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 1, 2022

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 1, 2022

a74nh added 2 commits November 2, 2022 15:30

Move phase and stop updating ssa

548dc4c

Wrap JitConfig access

5edc966

a74nh mentioned this pull request Nov 4, 2022

Assertion failed '!"Should not see phi nodes after rationalize"' during 'Lowering nodeinfo' #77886

Closed

a74nh added 4 commits November 4, 2022 12:16

Add GT_RETURN else cases

507cb1c

Add test cases with verification checks

1159d44

Ensure single only operation condition checks are used

9c83324

Merge main

b9cebb0

a74nh marked this pull request as draft November 7, 2022 17:11

Remove empty line

f857358

build-analysis bot mentioned this pull request Nov 7, 2022

Tracking issue for CI build timeouts #76454

Closed

a74nh marked this pull request as ready for review November 8, 2022 09:09

build-analysis bot mentioned this pull request Nov 9, 2022

Test failure JIT\\HardwareIntrinsics\\General\\Vector256\\Vector256_r\\Vector256_r.cmd #76280

Closed

a74nh added 10 commits November 14, 2022 14:59

Always invert condition

779cbfd

Rename IfConvertMergeBlocks

e6b2d1b

Use gtGetOp1()

cd4b580

Merge branch main

b96a719

Expand tests

e1c2f7e

Merge branch main

4ef54d4

Change-Id: I61c8419d32d10d005c746cc9ad6ee4e89320d015 CustomizedGitHooks: yes

Merge branch main

1a0927f

Add operation type assert

3a659e2

Allow nested SELECT nodes

211483d

Merge branch main

4a4a593

Change-Id: If61e778cc97e57dd20fd36ed623a1b0870c2b2bd CustomizedGitHooks: yes

a74nh mentioned this pull request Nov 18, 2022

Arm64: If Conversion can bypass overflow checks #78554

Closed

jakobbotsch mentioned this pull request Nov 23, 2022

JIT: Set GTF_ORDER_SIDEEFF for some nodes with invisible dependencies #78698

Merged

a74nh added 2 commits November 28, 2022 15:04

Merge branch main

355a3ff

Fix condition directions

0908788

jakobbotsch approved these changes Nov 29, 2022

View reviewed changes

jakobbotsch merged commit 5356857 into dotnet:main Nov 30, 2022

a74nh deleted the github_a74nh_csel_optimizer_else branch November 30, 2022 10:55

jakobbotsch mentioned this pull request Dec 6, 2022

Map correct fmt for mov instruction #79174

Merged

a74nh mentioned this pull request Dec 14, 2022

Arm64: Generate conditional comparison and selection instructions #55364

Closed

3 tasks

ghost locked as resolved and limited conversation to collaborators Dec 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simple Else conditions to If Conversion #77728

Add simple Else conditions to If Conversion #77728

a74nh commented Nov 1, 2022

ghost commented Nov 1, 2022

a74nh commented Nov 1, 2022

a74nh commented Nov 1, 2022

a74nh commented Nov 1, 2022

SingleAccretion commented Nov 1, 2022

kunalspathak commented Nov 2, 2022

a74nh commented Nov 2, 2022

SingleAccretion commented Nov 2, 2022

a74nh commented Nov 2, 2022

AndyAyersMS commented Nov 2, 2022

a74nh commented Nov 8, 2022

a74nh commented Nov 8, 2022

jakobbotsch commented Nov 8, 2022

jakobbotsch commented Nov 8, 2022

azure-pipelines bot commented Nov 8, 2022

a74nh commented Nov 16, 2022

jakobbotsch commented Nov 16, 2022 •

edited

Loading

a74nh commented Nov 16, 2022

a74nh commented Nov 29, 2022

jakobbotsch commented Nov 29, 2022

azure-pipelines bot commented Nov 29, 2022

jakobbotsch commented Nov 29, 2022

a74nh commented Nov 29, 2022

jakobbotsch commented Nov 30, 2022

jakobbotsch commented Nov 30, 2022

Add simple Else conditions to If Conversion #77728

Add simple Else conditions to If Conversion #77728

Conversation

a74nh commented Nov 1, 2022

ghost commented Nov 1, 2022

a74nh commented Nov 1, 2022

a74nh commented Nov 1, 2022

a74nh commented Nov 1, 2022

SingleAccretion commented Nov 1, 2022

kunalspathak commented Nov 2, 2022

a74nh commented Nov 2, 2022

SingleAccretion commented Nov 2, 2022

a74nh commented Nov 2, 2022

AndyAyersMS commented Nov 2, 2022

a74nh commented Nov 8, 2022

a74nh commented Nov 8, 2022

jakobbotsch commented Nov 8, 2022

jakobbotsch commented Nov 8, 2022

azure-pipelines bot commented Nov 8, 2022

a74nh commented Nov 16, 2022

jakobbotsch commented Nov 16, 2022 • edited Loading

a74nh commented Nov 16, 2022

a74nh commented Nov 29, 2022

jakobbotsch commented Nov 29, 2022

azure-pipelines bot commented Nov 29, 2022

jakobbotsch commented Nov 29, 2022

a74nh commented Nov 29, 2022

jakobbotsch commented Nov 30, 2022

jakobbotsch commented Nov 30, 2022

jakobbotsch commented Nov 16, 2022 •

edited

Loading