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 regions #971

Closed
wants to merge 34 commits into from
Closed

Verify regions #971

wants to merge 34 commits into from

Conversation

ushirask
Copy link
Contributor

@ushirask ushirask commented Mar 17, 2022 •
edited
Loading

Partially fixes #995

@ushirask ushirask marked this pull request as ready for review March 21, 2022 21:57
@ushirask ushirask requested a review from jclark as a code owner March 21, 2022 21:57
ushirask
ushirask commented Mar 22, 2022
View reviewed changes
compiler/modules/bir/verify.bal Outdated
check verifyNonFinalRegisterKind(vc, insn.operands[0]);
}
else if insn is MultipleOpeandInsn {
foreach var op in <Operand[]>insn.operands {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jclark without the type cast it throws the error
ERROR [verify.bal:(125:27,125:40)] incompatible types: '((wso2/nballerina.bir:0.1.0:IntOperand[2] & readonly)|(wso2/nballerina.bir:0.1.0:IntOperand[2] & readonly)|(wso2/nballerina.bir:0.1.0:IntOperand[2] & readonly)|(wso2/nballerina.bir:0.1.0:FloatOperand[2] & readonly)|(wso2/nballerina.bir:0.1.0:DecimalOperand[2] & readonly)|(wso2/nballerina.bir:0.1.0:Operand[2] & readonly)|(wso2/nballerina.bir:0.1.0:Operand[] & readonly)|([wso2/nballerina.bir:0.1.0:Register,wso2/nballerina.bir:0.1.0:IntOperand] & readonly)|(wso2/nballerina.bir:0.1.0:Operand[] & readonly)|([wso2/nballerina.bir:0.1.0:Register,wso2/nballerina.bir:0.1.0:StringOperand] & readonly)|(wso2/nballerina.bir:0.1.0:StringOperand[2] & readonly))' is not an iterable collection

This seems independent of the insn type. Even if we use just 2 instructions with same operands it still fails.
For type MultipleOpeandInsn ListConstructInsn|MappingConstructInsn;
incompatible types: '((wso2/nballerina.bir:0.1.0:Operand[] & readonly)|(wso2/nballerina.bir:0.1.0:Operand[] & readonly))' is not an iterable collection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an issue for JBUG in ballerina-platform/ballerina-lang#35557

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we work around by assigning to a variable before iterating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, changed it to assign and iterate.

@manuranga
Copy link
Contributor

Hi @jclark
Should we verify that a give tmp variables is used ones and only ones in a given branch? I think we do. If so, Ushira will need to take control flow into account.

@jclark
Copy link
Contributor

jclark commented Mar 24, 2022 •
edited
Loading

Hi @jclark Should we verify that a give tmp variables is used ones and only ones in a given branch? I think we do. If so, Ushira will need to take control flow into account.

For tmp, I was envisaging that we would check that it occurs as the result of exactly one instruction.

manuranga
manuranga reviewed Mar 30, 2022
View reviewed changes
compiler/modules/bir/verify.bal Outdated

function verifyRegistersKinds(VerifyContext vc, Insn insn) returns Error? {
if insn !is NonResultInsn {
vc.definedTmpRegisters.push(insn.result.number);
if vc.definedTmpRegisters.indexOf(insn.result.number, 0) != () {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for 0 its the defualt

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.

manuranga
manuranga reviewed Mar 30, 2022
View reviewed changes
compiler/modules/bir/verify.bal Outdated

function verifyNonFinalRegisterKind(VerifyContext vc, Operand r) returns Error? {
if r is FinalRegister {
return vc.invalidErr("invalid register kind final for insn: " + r.kind, <Position>r.pos);
Copy link
Contributor

Choose a reason for hiding this comment

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

too many spaces in indention.

jclark
jclark reviewed Apr 15, 2022
View reviewed changes
Copy link
Contributor

@jclark jclark left a comment

Choose a reason for hiding this comment

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

First of all, in the issue write a precise description of everything that you are checking.

compiler/modules/bir/verify.bal Outdated
}
}

type MultipleOpeandInsn IntBinaryInsn|IntNoPanicArithmeticBinaryInsn|FloatArithmeticBinaryInsn
Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling

compiler/modules/bir/verify.bal Outdated
|DecimalArithmeticBinaryInsn|CompareInsn|ListConstructInsn|ListGetInsn|MappingConstructInsn
|MappingGetInsn|StringConcatInsn|EqualityInsn;

type NonResultInsn MappingSetInsn|ListSetInsn|BranchInsn|CondBranchInsn|CondNarrowInsn|PanicInsn
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 it the other way round: ResultInsn.

compiler/modules/bir/verify.bal Outdated

function init(Module mod, FunctionDefn defn) {
final FunctionCode code;
int[] definedTmpRegisters = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

boolean[] tmpRegisterUsed

compiler/modules/bir/verify.bal Outdated
if r !is Register {
return;
}
if r is TmpRegister && vc.definedTmpRegisters.indexOf(r.number) == () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't want to make assumptions here about ordering of blocks.

@ushirask ushirask changed the title Verify register kinds Verify register kinds and regions Apr 21, 2022
@ushirask ushirask requested a review from jclark April 21, 2022 03:27
@jclark
Copy link
Contributor

jclark commented May 2, 2022

I can't review this until the description of the problem in #995 is fixed.

jclark
jclark requested changes May 2, 2022
View reviewed changes
Copy link
Contributor

@jclark jclark left a comment

Choose a reason for hiding this comment

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

Please fix the problems with the issue description in #995 first.

@ushirask ushirask mentioned this pull request May 9, 2022
Closed
@ushirask ushirask changed the title Verify register kinds and regions Verify regions May 10, 2022
@manuranga manuranga closed this Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manuranga manuranga manuranga left review comments

@jclark jclark jclark requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Verify register kinds and regions in BIR
3 participants
@ushirask @manuranga @jclark