Better detection of logical and/or inlining opportunities.

Better detection of logical and/or inlining opportunities.

Now allows the conditions to be moved by code motion or global-value numbering.
Also allow sequences of empty blocks with gotos between the branch and the
negation of an || construct.

Committed: https://code.google.com/p/dart/source/detail?r=6741

[ngeoffray, 2012-04-19 12:52:24]

LGTM!

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/codegen_helpers.dart (right):

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:184: HInstruction previous;
Unused variable?

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:193: if
(!generateAtUseSite.contains(instruction)) {
So this is the very first instruction of the block, right? Maybe add a comment.

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:197: if (block.phis.first
!== instruction) {
block.phis.length != 1 reads better

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:210: previousBlock.last is!
HGoto) {
Should that check be isBlockSinglePredecessor instead?

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:242: if (current is!
HControlFlow && !generateAtUseSite.contains(current)) {
Please add a comment on why you check that.

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:273: if (previousBlock.last
is HGoto) {
Use isBlockSinglePredecessor instead?

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:292: HBasicBlock
secondBlock = phi.block.predecessors[1];
How it was written before read better for me.

[Lasse Reichstein Nielsen, 2012-04-19 13:13:06]

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/codegen_helpers.dart (right):

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:184: HInstruction previous;
Yes, removed.

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:193: if
(!generateAtUseSite.contains(instruction)) {
On 2012/04/19 12:52:24, ngeoffray wrote:
> So this is the very first instruction of the block, right? Maybe add a
comment.

Done.
Also took care of the case when [instruction] started out as an HPhi.

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:197: if (block.phis.first
!== instruction) {
But runs worse. [block.phis] is a [HInstructionList] and doesn't have a length
property.

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:210: previousBlock.last is!
HGoto) {
It's more, since it also checks that the HControlFlow is an HGoto (probably
redundant).
Alas, I don't have access to isBlockSinglePredecessor here, it's in another
class.

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:242: if (current is!
HControlFlow && !generateAtUseSite.contains(current)) {
On 2012/04/19 12:52:24, ngeoffray wrote:
> Please add a comment on why you check that.

Done.

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:273: if (previousBlock.last
is HGoto) {
Don't have it. I don't think it's worth copying here, and half of what it checks
is already checked here (the predecessors.length == 1 check). I'll keep the test
for HGoto, just to be safe if we ever add another single-successor control flow
instruction.

https://chromiumcodereview.appspot.com/10126005/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen_helpers.dart:292: HBasicBlock
secondBlock = phi.block.predecessors[1];
Reordered.