Skip to content

Add conditional statements & expressions #8

New issue

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

#

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

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add conditional statements & expressions #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 35 additions & 0 deletions lib/cfg/constructor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ Constructor.prototype.visit = function visit(ast) {
return this.visitUpdate(ast);
else if (ast.type === 'SequenceExpression')
return this.visitSequence(ast);
else if (ast.type === 'IfStatement' || ast.type === 'ConditionalExpression')
return this.visitConditional(ast);
else
assert(false, `Unknown AST node type: "${ast.type}"`);
};
Expand Down Expand Up @@ -238,6 +240,39 @@ Constructor.prototype.visitSequence = function visitSequence(ast) {
return res;
};

Constructor.prototype.visitConditional = function visitConditional(ast) {
const test = this.visit(ast.test);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not move it closer to branch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, jump needs to be inserted after the test instructions but before branch to handle X in if (ternary condition). Or I might be missing something.

Copy link
Member

Choose a reason for hiding this comment

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

It can be right to the branch, that's fine. It is just a placement of instruction in a block, and it is way cleaner to place it closer to the branch that consumes it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I fail to see your idea on where it can be put. Can you please explain / show how the pipeline would look like for this test case?

if (true ? 1 : 2) 'x'; else 'y';

Copy link
Member

@indutny indutny Jun 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not suggesting to change anything significant, just suggesting to move the instruction to the start block. Right now, if you do a split at line 248 of the patch, this instruction will still be in a previous block:

if (a) {
} else {
}
if (b) {
} else {
}

Here something along the lines will be generated:

...
b3 {
  i0 = load b
}
b3 -> b4
b4 {
  i1 = test ^b4, i0
}
b4->b5, b6
...

While I think it is fine to have it in b4.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So back to this after long time :) The problem I see is that in order to do a split, you need this.pipeline.jumpFrom(start), so it depends on start, and in order to get start, you need to have already visited test. I would love to move visiting test to this newly created block, but not sure how to split that dependency (this.currentBlock doesn't seem to be as reliable replacement).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alternatively, I can split blocks unconditionally and thus drop dependency on start that we have in if (start.predecessors.length > 1) {, but it would lead to extraneous blocks.


let start = test.block;

if (start.predecessors.length > 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, fixing Xs in CFG ;)

this.pipeline.addControl('jump');
start = this.pipeline.jumpFrom(start);
}

const branch = this.pipeline.addControl('if', [ test ]);

const left = this.pipeline.jumpFrom(start);
const consequent = this.visit(ast.consequent);
this.pipeline.addControl('jump');

if (ast.alternate) {
const right = this.pipeline.jumpFrom(start);
const alternate = this.visit(ast.alternate);
this.pipeline.addControl('jump');

this.pipeline.merge(left, right);

if (ast.type === 'ConditionalExpression') {
return this.pipeline.addControl('phi', [ consequent, alternate ]);
}
} else {
this.pipeline.merge(left, start);
Copy link
Member

Choose a reason for hiding this comment

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

This is a bad idea, better create right block anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why? This allows to avoid a need to handle extra jumps.

}

return null;
};

//
// Routines
//
Expand Down
123 changes: 123 additions & 0 deletions test/constructor-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,4 +439,127 @@ describe('CFG.js/Constructor', () => {
}`);
});
});

describe('conditional', () => {
it('statement should be supported', () => {
test(() => {
if (true) {
'x';
} else {
'y';
}
}, `pipeline {
b0 {
i0 = literal true
i1 = if ^b0, i0
}
b0 -> b1, b2
b1 {
i2 = literal "x"
i3 = jump ^b1
}
b1 -> b3
b2 {
i4 = literal "y"
i5 = jump ^b2
}
b2 -> b3
b3 {
}
}`);
});

it('expression should evaluate', () => {
test(() => {
true ? 'x' : 'y';
}, `pipeline {
b0 {
i0 = literal true
i1 = if ^b0, i0
}
b0 -> b1, b2
b1 {
i2 = literal "x"
i3 = jump ^b1
}
b1 -> b3
b2 {
i4 = literal "y"
i5 = jump ^b2
}
b2 -> b3
b3 {
i6 = phi ^b3, i2, i4
}
}`);
});

it('statement without else should be supported', () => {
test(() => {
'a';
if (true) 'b';
'c';
}, `pipeline {
b0 {
i0 = literal "a"
i1 = literal true
i2 = if ^b0, i1
}
b0 -> b1, b2
b1 {
i3 = literal "b"
i4 = jump ^b1
}
b1 -> b2
b2 {
i5 = literal "c"
}
}`)
});

test(() => {
if (true ? 1 : 2) {
'x';
} else {
'y';
}
}, `pipeline {
b0 {
i0 = literal true
i1 = if ^b0, i0
}
b0 -> b1, b2
b1 {
i2 = literal 1
i3 = jump ^b1
}
b1 -> b3
b2 {
i4 = literal 2
i5 = jump ^b2
}
b2 -> b3
b3 {
i6 = phi ^b3, i2, i4
i7 = jump ^i6
}
b3 -> b4
b4 {
i8 = if ^b4, i6
}
b4 -> b5, b6
b5 {
i9 = literal "x"
i10 = jump ^b5
}
b5 -> b7
b6 {
i11 = literal "y"
i12 = jump ^b6
}
b6 -> b7
b7 {
}
}`);
});
});