Verify tmp registers #1010

ushirask · 2022-05-09T05:37:08Z

Purpose

Verify register kinds in BIR
Part of #995

Goals

Describe the solutions that this feature/fix will introduce to resolve the problems described above

Approach

Describe how you are implementing the solutions. Include an animated GIF or screenshot if the change affects the UI (email documentation@wso2.com to review all UI text). Include a link to a Markdown file or Google doc if the feature write-up is too long to paste here.

User stories

Summary of user stories addressed by this change>

Release note

Brief description of the new feature or bug fix as it will appear in the release notes

Documentation

Link(s) to product documentation that addresses the changes of this PR. If no doc impact, enter “N/A” plus brief explanation of why there’s no doc impact

Training

Link to the PR for changes to the training content in https://github.com/wso2/WSO2-Training, if applicable

Certification

Type “Sent” when you have provided new/updated certification questions, plus four answers for each question (correct answer highlighted in bold), based on this change. Certification questions/answers should be sent to certification@wso2.com and NOT pasted in this PR. If there is no impact on certification exams, type “N/A” and explain why.

Marketing

Link to drafts of marketing content that will describe and promote this feature, including product page changes, technical articles, blog posts, videos, etc., if applicable

Automation tests

Unit tests

Code coverage information
Integration tests

Details about the test cases and coverage

Security checks

Followed secure coding standards in http://wso2.com/technical-reports/wso2-secure-engineering-guidelines? yes/no
Ran FindSecurityBugs plugin and verified report? yes/no
Confirmed that this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets? yes/no

Samples

Provide high-level details about the samples related to this feature

Related PRs

List any other related PRs

Migrations (if applicable)

Describe migration steps and platforms on which migration has been tested

Test environment

List all JDK versions, operating systems, databases, and browser/versions on which this feature/fix was tested

Learning

Describe the research phase and any blog posts, patterns, libraries, or add-ons you used to solve the problem.

jclark · 2022-05-11T04:59:58Z

compiler/modules/bir/verify.bal

@@ -10,12 +10,15 @@ class VerifyContext {
    private final Module mod;
    private final t:Context tc;
    private final FunctionDefn defn;
+    final FunctionCode code;
+    boolean[] tmpRegisterUsed = [];


tmpRegisterUsed looks like it's final too.

I suggest you make both of them private, and then add methods to VerifyContext to encapsulate how you use them.

can we make tmpRegisterUsed final since we modify the boolean values?

jclark · 2022-05-11T05:02:31Z

compiler/modules/bir/verify.bal

+    |DecimalArithmeticBinaryInsn|CompareInsn|ListConstructInsn|ListGetInsn|MappingConstructInsn
+    |MappingGetInsn|StringConcatInsn|EqualityInsn;
+
+type ResultInsn IntArithmeticBinaryInsn|IntNoPanicArithmeticBinaryInsn|IntBitwiseBinaryInsn


Might be simpler to do something like readonly & record { *ResultInsnBase; readonly ... } (if jBallerina accepts it)

manuranga · 2022-05-18T17:44:26Z

compiler/modules/bir/verify.bal

@@ -10,6 +10,7 @@ class VerifyContext {
    private final Module mod;
    private final t:Context tc;
    private final FunctionDefn defn;
+    private final boolean[] tmpRegisterUsed = [];


I would just pass it as a param and not make VC stateful.

manuranga · 2022-05-18T17:51:03Z

compiler/modules/bir/verify.bal

+        Register narrowed = r.underlying;
+        while narrowed is NarrowRegister {
+            narrowed = narrowed.underlying;
+        }
+        if !t:isSubtype(vc.typeContext(), r.semType, narrowed.semType) {
+            return vc.invalidErr("narrow register is not a subtype of narrowed register", <Position>r.pos);
+        }


I don't think we need to check the every time we read form a NarrowRegister, just checking when we first narrow (ie: TypeMerge/TypeBr) is enough. That overlaps with my work.

compiler/modules/bir/verify.bal

manuranga · 2022-05-18T18:33:08Z

compiler/modules/bir/verify.bal

+type ResultInsn readonly & record { *ResultInsnBase; };
+
+function verifyInsnRegisterKinds(VerifyContext vc, Insn insn) returns Error? {
+    if insn is ResultInsn {


Can we use if insn is ResultInsnBase { instead?

manuranga · 2022-05-18T18:42:50Z

compiler/modules/bir/verify.bal

+
+function verifyInsnRegisterKinds(VerifyContext vc, Insn insn) returns Error? {
+    if insn is ResultInsn {
+        if (vc.getNumTmpRegisters() > insn.result.number) && vc.getUsedTmpRegisters(insn.result.number) {


if tmpRegisterUsed[insn.result.number] {

Pre-populate the tmpRegisterUsed array with false upto registers.length(). Then you don't have to check the length.

manuranga · 2022-05-19T06:29:43Z

compiler/modules/bir/verify.bal

+    if r is FinalRegister {
+        return vc.invalidErr("invalid register kind final for insn: " + r.kind, <Position>r.pos);
+    }


Can we do this check at verifyListSet/verifyMappingSet?

manuranga · 2022-05-19T06:49:09Z

compiler/modules/bir/verify.bal

+    if insn is BranchInsn|CatchInsn {
+        return;
+    }
+    else if insn is CallInsn {
+        foreach Operand arg in insn.args {
+            check verifyRegisterKind(vc, arg, tmpRegisterUsed, insn.pos);
+        }
+    }


Move this to match and get rid of the if

manuranga · 2022-05-19T07:16:24Z

compiler/modules/bir/verify.bal

+        var { operand } => {
+            check verifyRegisterKind(vc, operand, tmpRegisterUsed, insn.pos);
+        }
+        var { operands } => {


Can we do var { operands } | { args: operands } => { not sure if it works in jBallerina, if not I think a issue needs to be created.

Anyway I think CallInsn needs to be changed to have operands instead of args for consistency. Please also create an issue for that.

We can do var { operands } | var { args: operands } => {

manuranga · 2022-05-19T07:31:10Z

compiler/modules/bir/verify.bal

+        if tmpRegisterUsed[insn.result.number] {
+            return vc.invalidErr("tmp register defined in multiple places", insn.pos);


I think we should call this tmpRegisterInit. defined doesn't sound right. Can we say multiple assignments to a tmp register ?

manuranga · 2022-05-19T07:43:27Z

compiler/modules/bir/verify.bal

+    }
+}
+
+function verifyRegisterKind(VerifyContext vc, Operand r, boolean[] tmpRegisterInit, Position pos) returns Error? {


Current name doesn't explain what it does. How about verifyTmpOprandInitialized

manuranga · 2022-05-19T08:34:17Z

compiler/modules/bir/verify.bal

+    }
+}
+
+function verifyTmpOprandInitialized(VerifyContext vc, Operand r, boolean[] tmpRegisterInit, Position pos) returns Error? {


verifyTmpOperandInitialized

manuranga · 2022-05-19T08:35:12Z

compiler/modules/bir/verify.bal

@@ -67,18 +67,59 @@ class VerifyContext {

 public function verifyFunctionCode(Module mod, FunctionDefn defn, FunctionCode code) returns Error? {
    VerifyContext cx = new(mod, defn);
+    boolean[] tmpRegisterInit = [];


On a second though, let's call this tmpRegisterInitialized to match with the func name.

manuranga · 2022-05-19T08:36:45Z

compiler/modules/bir/verify.bal

+    }
+}
+
+function verifyInsnRegisterKinds(VerifyContext vc, Insn insn, boolean[] tmpRegisterInit) returns Error? {


verifyTmpRegisters

manuranga · 2022-05-19T11:33:16Z

compiler/modules/bir/verify.bal

+    if insn.operands[0] is FinalRegister {
+        return vc.invalidErr("invalid register kind final for ListSetInsn", insn.pos);
+    }


This is wrong.

import ballerina/io; public function main() { final int[] i = [1, 2, 3]; i[0] = 4; io:println(i); }

Above is a valid Ballerina program.

manuranga · 2022-05-19T11:54:45Z

compiler/modules/bir/verify.bal

+    if insn.operands[0] is FinalRegister {
+        return vc.invalidErr("invalid register kind final for MappingSetInsn", insn.pos);
+    }


Same as above

manuranga · 2022-05-25T05:46:47Z

compiler/modules/front/stmt.bal

+        if matched is bir:TmpRegister {
+            bir:AssignTmpRegister result = cx.createAssignTmpRegister(matched.semType, matched.pos);
+            bir:AssignInsn insn = { result, operand: matched, pos : stmt.startPos };
+            matched = result;


In a match we are only reading matched, aren't we ? why do we need AssignTmpRegister?

In a case like

match x + 1 { 2 => { return "1"; } 4 => { return "2"; } }

even though matched is a TmpRegister it will be used in multiple blocks for Equality instructions, so to avoid that I think we can either do something like this or change all expr code gens to create AssignTmpRegister instead of Tmp depending on the situation.

I think we'll have to change the definition of TmpRegister to, assigned once, but can be used form multiple blocks.
@jclark what do you think?

I believe this is already the case, the problem is when the TmpRegister is used in a different block than the block it was initially assigned.

but can be used multiple times

I meant

but can be used from multiple block

let me edit.

manuranga · 2022-05-26T05:51:29Z

@jclark this is ready for you to review.

manuranga · 2022-05-26T05:52:36Z

compiler/modules/bir/verify.bal

+    }
+}
+
+function verifyTmpRegisters(VerifyContext vc, Insn insn, boolean[] tmpRegisterInitialized) returns Error? {


let's call this verifyTmpRegisterAssignment

jclark · 2022-06-16T04:18:18Z

compiler/modules/bir/verify.bal

@@ -67,18 +67,34 @@ class VerifyContext {

 public function verifyFunctionCode(Module mod, FunctionDefn defn, FunctionCode code) returns Error? {
    VerifyContext cx = new(mod, defn);
+    boolean[] tmpRegisterInitialized = [];
+    if code.registers.length() > 0 {


Cleaner to do tmpRegisterIntialized.setLength(code.registers.length());

jclark · 2022-06-16T04:18:49Z

compiler/modules/bir/verify.bal

+
+function verifyTmpRegisterAssignment(VerifyContext vc, Insn insn, boolean[] tmpRegisterInitialized) returns Error? {
+    if insn is ResultInsnBase {
+        if tmpRegisterInitialized[insn.result.number] {


Add int num = insn.result.number;

Verify register kinds

4d7932a

ushirask requested a review from jclark as a code owner May 9, 2022 05:37

Rename function

11f3509

jclark requested changes May 11, 2022

View reviewed changes

ushirask added 5 commits May 13, 2022 09:38

make variables private

9318029

Merge branch 'ballerina-platform:main' into 995_register

753c457

fix conflicts

abdbf53

Refactor code

3a4c0df

Merge branch 'ballerina-platform:main' into 995_register

8eba31b

manuranga reviewed May 18, 2022

View reviewed changes

compiler/modules/bir/verify.bal Outdated Show resolved Hide resolved

manuranga reviewed May 18, 2022

View reviewed changes

ushirask added 3 commits May 19, 2022 09:48

Refactor code

f2a1382

Use match statment

e2ec714

Remove unused param

d6879e7

ushirask requested a review from manuranga May 19, 2022 06:02

manuranga reviewed May 19, 2022

View reviewed changes

Refactor code

4d12ef0

manuranga reviewed May 19, 2022

View reviewed changes

Refactor code

54ce38f

manuranga reviewed May 19, 2022

View reviewed changes

Refactor code

87e1184

manuranga reviewed May 19, 2022

View reviewed changes

Rename to tmpRegisterInit

0b1638e

manuranga reviewed May 19, 2022

View reviewed changes

Rename function

06d93f7

manuranga reviewed May 19, 2022

View reviewed changes

Rename param

d16e0b2

manuranga reviewed May 19, 2022

View reviewed changes

ushirask added 2 commits May 20, 2022 09:41

Remove incorrect verification

a5b3a3f

Remove incorrect verification

9546bee

ushirask requested a review from manuranga May 23, 2022 03:39

ushirask added 2 commits May 23, 2022 11:47

Move TypeMerge/Branch checks

82cfbb3

Fix match stmt assign tmp

59b6c9f

manuranga reviewed May 25, 2022

View reviewed changes

Fix foreach stmt assign tmp

88838a1

manuranga changed the title ~~Verify register kinds~~ Verify tmp registers May 25, 2022

ushirask added 2 commits May 26, 2022 10:52

Remove tmp register read check

2895ca3

Remove unused function

f264fd5

ushirask requested a review from manuranga May 26, 2022 05:25

manuranga requested a review from jclark May 26, 2022 05:51

manuranga reviewed May 26, 2022

View reviewed changes

Rename function

029c548

jclark requested changes Jun 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify tmp registers #1010

Verify tmp registers #1010

ushirask commented May 9, 2022 •

edited by manuranga

jclark May 11, 2022

ushirask May 12, 2022 •

edited

jclark May 11, 2022

manuranga May 18, 2022

manuranga May 18, 2022

manuranga May 18, 2022

manuranga May 18, 2022

manuranga May 19, 2022

manuranga May 19, 2022

manuranga May 19, 2022 •

edited

ushirask May 19, 2022

manuranga May 19, 2022

ushirask May 19, 2022

manuranga May 19, 2022

manuranga May 19, 2022

manuranga May 19, 2022

manuranga May 19, 2022 •

edited

manuranga May 19, 2022

manuranga May 19, 2022

manuranga May 25, 2022

ushirask May 25, 2022

manuranga May 25, 2022 •

edited

ushirask May 25, 2022

manuranga May 25, 2022 •

edited

manuranga commented May 26, 2022

manuranga May 26, 2022

jclark Jun 16, 2022

jclark Jun 16, 2022

		if tmpRegisterUsed[insn.result.number] {
		return vc.invalidErr("tmp register defined in multiple places", insn.pos);

Verify tmp registers #1010

Are you sure you want to change the base?

Verify tmp registers #1010

Conversation

ushirask commented May 9, 2022 • edited by manuranga

Purpose

Goals

Approach

User stories

Release note

Documentation

Training

Certification

Marketing

Automation tests

Security checks

Samples

Related PRs

Migrations (if applicable)

Test environment

Learning

Choose a reason for hiding this comment

ushirask May 12, 2022 • edited

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

manuranga May 19, 2022 • edited

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

manuranga May 19, 2022 • edited

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

manuranga May 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuranga May 25, 2022 • edited

Choose a reason for hiding this comment

manuranga commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ushirask commented May 9, 2022 •

edited by manuranga

ushirask May 12, 2022 •

edited

manuranga May 19, 2022 •

edited

manuranga May 19, 2022 •

edited

manuranga May 25, 2022 •

edited

manuranga May 25, 2022 •

edited