[mono][interp] Expand bblocks reordering with common patterns to facilitate propagation of values #80362

kotlarmilos · 2023-01-09T10:12:02Z

This PR expands bblocks reordering with the following common patterns:

branch: a branch instruction (either MINT_IS_CONDITIONAL_BRANCH or MINT_BR).
ldc; branch: a load instruction followed by a binary conditional branch.
ldc; compare; branch: a load instruction followed by a compare instruction and a unary conditional branch.

For example, the following pattern could be inlined into callers bblocks:

BB8:
     ldc.i4.0 [var1 <- nil],
     br.s BB1
BB5:
     ldc.i4.1 [var2 <- nil],
     cgt.i4 [var3 <- var1 var2],
     brfalse.i4.s [nil <- var3] BB2
BB3:
     ldstr

This PR reorders the code to look like:

BB8:
     ldc.i4.0 [var1 <- nil],
     ldc.i4.1 [var2 <- nil],
     cgt.i4 [var3 <- var1 var2],
     brfalse.i4.s [nil <- var3] BB2
BB11:
     br             [nil <- nil], BB3

The code is further optimized by constants folding and superins.

Draft PR should confirm if the runtime works as expected.

…o callers bblocks. The known patterns are: branch: a single conditional or unconditional branch instruction. ldc; branch or compare; branch: a load instruction or compare instruction followed by a conditional or unconditional branch instruction. ldc; compare; branch: a load instruction followed by a compare instruction and a conditional or unconditional branch instruction.

…riables only

ghost · 2023-01-09T10:12:18Z

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

Issue Details

This PR expands bblocks reordering with the following common patterns:

branch: a branch instruction (either MINT_IS_CONDITIONAL_BRANCH or MINT_BR).
ldc; branch: a load instruction followed by a binary conditional branch.
compare; branch: a compare instruction followed by a unary conditional branch.
ldc; compare; branch: a load instruction followed by a compare instruction and a unary conditional branch.

For example, the following pattern could be inlined into callers bblocks:

BB8:
     ldc.i4.0 [var1 <- nil],
     br.s BB1
BB5:
     ldc.i4.1 [var2 <- nil],
     cgt.i4 [var3 <- var1 var2],
     brfalse.i4.s [nil <- var3] BB2
BB3:
     ldstr

This PR reorders the code to look like:

BB8:
     ldc.i4.0 [var1 <- nil],
     ldc.i4.1 [var2 <- nil],
     cgt.i4 [var3 <- var1 var2],
     brfalse.i4.s [nil <- var3] BB2
BB11:
     br             [nil <- nil], BB3

The code is further optimized by constants folding and superins.

Author:	kotlarmilos
Assignees:	kotlarmilos
Labels:	`area-Codegen-Interpreter-mono`
Milestone:	8.0.0

BrzVlad · 2023-01-09T12:31:08Z

src/mono/mono/mini/interp/transform.c

@@ -8438,16 +8538,16 @@ interp_reorder_bblocks (TransformData *td)
 					i++;
 				}
 			}
-		} else if (first->opcode == MINT_BR) {


Why is there a change to this separate optimization here ? We can also get rid of special casing MINT_BR in interp_inline_into_callers

No need, reverted.

BrzVlad · 2023-01-09T12:45:04Z

src/mono/mono/mini/interp/transform.c

+	InterpInst *first = interp_first_ins (bb);
+	InterpInst *second = first ? interp_next_ins(first) : NULL;
+	InterpInst *third = second ? interp_next_ins(second) : NULL;
+


Maybe compiler is smart enough. But you could avoid checking for non null with every single if like:

if (!first) return; ... checking patterns with first instruction ... if (!second) return; ... checking patterns with first and second instructions ...

Good point, updated.

BrzVlad · 2023-01-09T12:50:27Z

src/mono/mono/mini/interp/transform.c

+ * - `compare; branch`: a compare instruction followed by a unary conditional branch.
+ * - `ldc; compare; branch`: a load instruction followed by a compare instruction and a unary conditional branch.
+ */
+static gboolean


I think the return value of cond_ins is rather unnecessary. It could instead directly return the lookup vars.

Good point, updated.

BrzVlad · 2023-01-09T12:53:19Z

src/mono/mono/mini/interp/transform.c

+					InterpInst *dest_ins = interp_prev_ins(last_ins);
+
+					// Copy a pattern into caller bb
+					while (src_ins) {


This copying here seems a bit hard to follow and unnatural. We could instead just copy all instructions one by one, from interp_first_ins (bb) to the end of the in_bb (after we got rid of the ending MINT_BR).

Updated. When copying instructions,MINT_LDC_I4 has ins->data [1] variable and there is a conditional statement for such cases. Other option is to iterate through ins->data but it could make the runtime slower.

… Copy a branch pattern into callers bblocks.

BrzVlad · 2023-01-10T10:05:19Z

src/mono/mono/mini/interp/transform.c

+	if (!second) 
+		return NULL;
+	// pattern `ldc; unop conditional branch`
+	if (MINT_IS_LDC(first->opcode)


This pattern is redundant and it is probably not reached anyway. ldc + brtrue/false is already resolved to br.s or optimized out.

Good point. Removed.

BrzVlad · 2023-01-10T10:15:28Z

src/mono/mono/mini/interp/transform.c

+		return second;
+	}
+	// pattern `compare; unop conditional branch`
+	if (MINT_IS_COMPARE(first->opcode)


Do you have a C# example where this case leads to better codegen ? I think we should stick to cases that we can see in practice, and not overdo it with duplicating code.

I don't have such example. Agree, keep it simple and clean.

BrzVlad · 2023-01-11T16:35:58Z

src/mono/mono/mini/interp/transform.c

 {
+	InterpInst *prev_ins = interp_prev_ins(cond_ins);
+	int var1 = -1, var2 = -1;


I feel like declaring the lookup vars here is not really the best. It doesn't correctly (strictly speaking) initializes the lookup vars. For example in some of the patterns, we know the value of one of the sregs from the existing bblock (ldc + compare/condbr), in which case we should only look for the unkown var. Also we can set these lookup vars directly in interp_inline_into_callers, as we check the patterns, without having these separate checks.

Good idea, updated.

BrzVlad · 2023-01-11T16:37:37Z

as with the other PR there is a lot of missing space before (

kotlarmilos · 2023-01-11T17:22:09Z

as with the other PR there is a lot of missing space before (

Thanks for the heads up. Failures seem to be relevant.

…otlarmilos/runtime into improvement/inline-branch-into-callers

BrzVlad · 2023-01-14T09:51:26Z

src/mono/mono/mini/interp/transform.c

+					while (copy_ins) {
+						InterpInst *new_ins = interp_insert_ins_bb (td, in_bb, in_bb->last_ins, copy_ins->opcode);
+						new_ins->dreg = copy_ins->dreg;
+						new_ins->sregs[0] = copy_ins->sregs[0];


space before [

BrzVlad · 2023-01-14T09:51:36Z

src/mono/mono/mini/interp/transform.c

+						new_ins->dreg = copy_ins->dreg;
+						new_ins->sregs[0] = copy_ins->sregs[0];
+						if (mono_interp_op_sregs[copy_ins->opcode] > 1)
+							new_ins->sregs[1] = copy_ins->sregs[1];


space before [

BrzVlad · 2023-01-14T09:54:39Z

src/mono/mono/mini/interp/transform.c

@@ -8441,7 +8511,7 @@ interp_reorder_bblocks (TransformData *td)
 				}
 			}
 		} else if (first->opcode == MINT_BR) {
-			// All bblocks jumping into this bblock can jump directly into the br target
+			// All bblocks jumping into this bblock can jump directly into the br target only if it is a single instruction in a bb


The comment addition seems confusing. Did you mean to say since it is the single instruction of the bb ?

Updated. It was confusing and left over from previous changes.

kotlarmilos added 4 commits January 7, 2023 15:08

remove printf

1b8bcb3

Simplify interp_prev_block_defines_var to check compare and branch va…

fe15753

…riables only

Update interp_inline_into_callers function to match specific patterns

f3e68f1

kotlarmilos added this to the 8.0.0 milestone Jan 9, 2023

kotlarmilos self-assigned this Jan 9, 2023

dotnet-issue-labeler bot added the area-Codegen-Interpreter-mono label Jan 9, 2023

kotlarmilos changed the title ~~Expand bblocks reordering with common patterns to facilitate propagation of values~~ [mono][interp] Expand bblocks reordering with common patterns to facilitate propagation of values Jan 9, 2023

BrzVlad reviewed Jan 9, 2023

View reviewed changes

runfoapp bot mentioned this pull request Jan 9, 2023

Infra improvements for Helix #68176

Closed

build-analysis bot mentioned this pull request Jan 9, 2023

Tracking issue for CI build timeouts #76454

Closed

Update interp_inline_into_callers function to return lookup cond_ins.…

19d235f

… Copy a branch pattern into callers bblocks.

kotlarmilos marked this pull request as ready for review January 9, 2023 22:53

kotlarmilos requested a review from vargaz as a code owner January 9, 2023 22:53

build-analysis bot mentioned this pull request Jan 10, 2023

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

5 tasks

BrzVlad reviewed Jan 10, 2023

View reviewed changes

Remove ldc; unop conditional branch and compare; branch patterns

6c3d864

kotlarmilos mentioned this pull request Jan 10, 2023

Tracking issue for CI build failure "Installing openssl@3 has failed!" #80428

Closed

Merge branch 'dotnet:main' into improvement/inline-branch-into-callers

ce71ad4

BrzVlad reviewed Jan 11, 2023

View reviewed changes

kotlarmilos added 3 commits January 12, 2023 13:25

Set lookup vars in interp_inline_into_callers

4c1ec5c

Merge branch 'improvement/inline-branch-into-callers' of github.com:k…

3b4f860

…otlarmilos/runtime into improvement/inline-branch-into-callers

add new line

54075a7

Use LDC_I4 in match pattern

5f680e8

BrzVlad reviewed Jan 14, 2023

View reviewed changes

Update styling and comment

a5742a1

BrzVlad approved these changes Jan 16, 2023

View reviewed changes

kotlarmilos merged commit c0447bc into dotnet:main Jan 16, 2023

kotlarmilos deleted the improvement/inline-branch-into-callers branch January 16, 2023 13:40

ghost locked as resolved and limited conversation to collaborators Feb 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono][interp] Expand bblocks reordering with common patterns to facilitate propagation of values #80362

[mono][interp] Expand bblocks reordering with common patterns to facilitate propagation of values #80362

kotlarmilos commented Jan 9, 2023 •

edited

Loading

ghost commented Jan 9, 2023

BrzVlad Jan 9, 2023 •

edited

Loading

kotlarmilos Jan 9, 2023

BrzVlad Jan 9, 2023

kotlarmilos Jan 9, 2023

BrzVlad Jan 9, 2023

kotlarmilos Jan 9, 2023

BrzVlad Jan 9, 2023

kotlarmilos Jan 9, 2023

BrzVlad Jan 10, 2023

kotlarmilos Jan 10, 2023

BrzVlad Jan 10, 2023

kotlarmilos Jan 10, 2023

BrzVlad Jan 11, 2023 •

edited

Loading

kotlarmilos Jan 13, 2023

BrzVlad commented Jan 11, 2023

kotlarmilos commented Jan 11, 2023

BrzVlad Jan 14, 2023

BrzVlad Jan 14, 2023

BrzVlad Jan 14, 2023 •

edited

Loading

kotlarmilos Jan 16, 2023

[mono][interp] Expand bblocks reordering with common patterns to facilitate propagation of values #80362

[mono][interp] Expand bblocks reordering with common patterns to facilitate propagation of values #80362

Conversation

kotlarmilos commented Jan 9, 2023 • edited Loading

ghost commented Jan 9, 2023

BrzVlad Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrzVlad Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrzVlad commented Jan 11, 2023

kotlarmilos commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrzVlad Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotlarmilos commented Jan 9, 2023 •

edited

Loading

BrzVlad Jan 9, 2023 •

edited

Loading

BrzVlad Jan 11, 2023 •

edited

Loading

BrzVlad Jan 14, 2023 •

edited

Loading