Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Verify tmp registers #1010

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Verify tmp registers #1010

Changes from 12 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
4d7932a
Verify register kinds
ushirask May 9, 2022
11f3509
Rename function
ushirask May 9, 2022
9318029
make variables private
ushirask May 13, 2022
753c457
Merge branch 'ballerina-platform:main' into 995_register
ushirask May 13, 2022
abdbf53
fix conflicts
ushirask May 13, 2022
3a4c0df
Refactor code
ushirask May 18, 2022
8eba31b
Merge branch 'ballerina-platform:main' into 995_register
ushirask May 18, 2022
f2a1382
Refactor code
ushirask May 19, 2022
e2ec714
Use match statment
ushirask May 19, 2022
d6879e7
Remove unused param
ushirask May 19, 2022
4d12ef0
Refactor code
ushirask May 19, 2022
54ce38f
Refactor code
ushirask May 19, 2022
87e1184
Refactor code
ushirask May 19, 2022
0b1638e
Rename to tmpRegisterInit
ushirask May 19, 2022
06d93f7
Rename function
ushirask May 19, 2022
d16e0b2
Rename param
ushirask May 19, 2022
a5b3a3f
Remove incorrect verification
ushirask May 20, 2022
9546bee
Remove incorrect verification
ushirask May 20, 2022
82cfbb3
Move TypeMerge/Branch checks
ushirask May 23, 2022
59b6c9f
Fix match stmt assign tmp
ushirask May 25, 2022
88838a1
Fix foreach stmt assign tmp
ushirask May 25, 2022
2895ca3
Remove tmp register read check
ushirask May 26, 2022
f264fd5
Remove unused function
ushirask May 26, 2022
029c548
Rename function
ushirask May 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 62 additions & 2 deletions compiler/modules/bir/verify.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,18 +67,65 @@ class VerifyContext {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tmpRegisterUsed[code.registers.length() - 1] = false;
}
foreach BasicBlock b in code.blocks {
check verifyBasicBlock(cx, b);
check verifyBasicBlock(cx, b, tmpRegisterUsed);
}
}

type IntBinaryInsn IntArithmeticBinaryInsn|IntBitwiseBinaryInsn;

type Error err:Semantic|err:Internal;

function verifyBasicBlock(VerifyContext vc, BasicBlock bb) returns Error? {
function verifyBasicBlock(VerifyContext vc, BasicBlock bb, boolean[] tmpRegisterUsed) returns Error? {
foreach Insn insn in bb.insns {
check verifyInsn(vc, insn);
check verifyInsnRegisterKinds(vc, insn, tmpRegisterUsed);
}
}

function verifyInsnRegisterKinds(VerifyContext vc, Insn insn, boolean[] tmpRegisterUsed) returns Error? {
if insn is ResultInsnBase {
if tmpRegisterUsed[insn.result.number] {
return vc.invalidErr("tmp register defined in multiple places", insn.pos);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}
else {
tmpRegisterUsed[insn.result.number] = true;
}
}

match insn {
var { operand } => {
check verifyRegisterKind(vc, operand, tmpRegisterUsed, insn.pos);
}
var { operands } => {
Copy link
Contributor

@manuranga manuranga May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// JBUG #35557 can't iterate operands
Operand[] ops = operands;
foreach Operand op in ops {
check verifyRegisterKind(vc, op, tmpRegisterUsed, insn.pos);
}
}
var { args } => {
Operand[] ops = args;
foreach Operand op in ops {
check verifyRegisterKind(vc, op, tmpRegisterUsed, insn.pos);
}
}
}
}

function verifyRegisterKind(VerifyContext vc, Operand r, boolean[] tmpRegisterUsed, Position pos) returns Error? {
if r is TmpRegister && !tmpRegisterUsed[r.number] {
return vc.invalidErr("tmp register not initialized", pos);
}
}

function verifyNarrowRegister(VerifyContext vc, NarrowRegister r) returns Error? {
if !t:isSubtype(vc.typeContext(), r.semType, r.underlying.semType) {
return vc.invalidErr("narrow register is not a subtype of narrowed register", <Position>r.pos);
}
}

Expand Down Expand Up @@ -151,6 +198,13 @@ function verifyInsn(VerifyContext vc, Insn insn) returns Error? {
else if insn is ErrorConstructInsn {
check validOperandString(vc, name, insn.operand, insn.pos);
}
else if insn is TypeMergeInsn {
check verifyNarrowRegister(vc, insn.result);
}
else if insn is TypeBranchInsn {
check verifyNarrowRegister(vc, insn.ifTrueRegister);
check verifyNarrowRegister(vc, insn.ifFalseRegister);
}
}

function verifyCall(VerifyContext vc, CallInsn insn) returns err:Internal? {
Expand Down Expand Up @@ -226,6 +280,9 @@ function verifyListSet(VerifyContext vc, ListSetInsn insn) returns Error? {
if !vc.isSubtype(insn.operands[0].semType, t:LIST) {
return vc.semanticErr("list set applied to non-list", insn.pos);
}
if insn.operands[0] is FinalRegister {
return vc.invalidErr("invalid register kind final for ListSetInsn", insn.pos);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

t:SemType memberType = t:listMemberType(vc.typeContext(), insn.operands[0].semType, insn.operands[1].semType);
return verifyOperandType(vc, insn.operands[2], memberType, "value assigned to member of list is not a subtype of array member type", insn.pos);
}
Expand All @@ -251,6 +308,9 @@ function verifyMappingSet(VerifyContext vc, MappingSetInsn insn) returns Error?
if !vc.isSubtype(insn.operands[0].semType, t:MAPPING) {
return vc.semanticErr("mapping set applied to non-mapping", insn.pos);
}
if insn.operands[0] is FinalRegister {
return vc.invalidErr("invalid register kind final for MappingSetInsn", insn.pos);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

t:SemType memberType = t:mappingMemberType(vc.typeContext(), insn.operands[0].semType, keyOperand.semType);
return verifyOperandType(vc, insn.operands[2], memberType, "value assigned to member of mapping is not a subtype of map member type", insn.pos);
}
Expand Down