Subzero: Fix Phi lowering.

Subzero: Fix Phi lowering.

This addresses a TODO and fixes a code generation bug that was causing sprintf and _vfprintf_r to execute incorrectly.

BUG= none
R=jvoung@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=e5a5be7

[Jim Stichnoth, 2014-09-10 16:50:03]

stichnot@chromium.org changed reviewers:
+ jvoung@chromium.org

[jvoung (off chromium), 2014-09-10 17:17:43]

https://codereview.chromium.org/560773002/diff/1/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/560773002/diff/1/src/IceCfgNode.cpp#newcode155
src/IceCfgNode.cpp:155: // an llvm_unreachable().
Maybe this confirmation should be earlier than the comment about
SafeInsertionPoint (seems to be a continuation of // Every block must end in a
terminator instruction.)

https://codereview.chromium.org/560773002/diff/1/tests_lit/llvm2ice_tests/phi.ll
File tests_lit/llvm2ice_tests/phi.ll (right):

https://codereview.chromium.org/560773002/diff/1/tests_lit/llvm2ice_tests/phi...
tests_lit/llvm2ice_tests/phi.ll:32: ; CHECK: cmp {{.*}}, 0
Would it be clearer to check for another jump? (that's what happens when fusing
doesn't happen right?)

https://codereview.chromium.org/560773002/diff/1/tests_lit/llvm2ice_tests/phi...
tests_lit/llvm2ice_tests/phi.ll:45: %merge = phi i32 [ %arg, %entry ], [ 0,
%next ]
If you change the %arg to a recognizable constant, would that make it easier to
check for the assignment before the cmp?

https://codereview.chromium.org/560773002/diff/1/tests_lit/llvm2ice_tests/phi...
tests_lit/llvm2ice_tests/phi.ll:51: ; CHECK: jg
Should this be CHECK-NEXT?

[Jim Stichnoth, 2014-09-10 18:29:16]

https://codereview.chromium.org/560773002/diff/1/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/560773002/diff/1/src/IceCfgNode.cpp#newcode155
src/IceCfgNode.cpp:155: // an llvm_unreachable().
On 2014/09/10 17:17:43, jvoung wrote:
> Maybe this confirmation should be earlier than the comment about
> SafeInsertionPoint (seems to be a continuation of // Every block must end in a
> terminator instruction.)

Actually, the "Every block must..." comment was meant to say that every block
must have at least one instruction and therefore it's valid to decrement
InsertionPoint.

But you're right and it seems better to do the validation before the long-winded
comment.

https://codereview.chromium.org/560773002/diff/1/tests_lit/llvm2ice_tests/phi.ll
File tests_lit/llvm2ice_tests/phi.ll (right):

https://codereview.chromium.org/560773002/diff/1/tests_lit/llvm2ice_tests/phi...
tests_lit/llvm2ice_tests/phi.ll:32: ; CHECK: cmp {{.*}}, 0
On 2014/09/10 17:17:43, jvoung wrote:
> Would it be clearer to check for another jump? (that's what happens when
fusing
> doesn't happen right?)

The problem I had is that the current code generation looks like this:

	cmp	al, 0
	jne	.LtestPhi1$next
	jmp	.LtestPhi1$target
.LtestPhi1$next:
	mov	cl, 0
	jmp	.LtestPhi1$target
.LtestPhi1$target:

After we fix the excessive number of branches, it should look more like this:

	cmp	al, 0
	je	.LtestPhi1$target
	mov	cl, 0
.LtestPhi1$target:

So today's test for jne would break after this change.

However, after writing this up, I realize that my test for two labels will also
break, so yeah, let's make it clearer for now. :)

https://codereview.chromium.org/560773002/diff/1/tests_lit/llvm2ice_tests/phi...
tests_lit/llvm2ice_tests/phi.ll:45: %merge = phi i32 [ %arg, %entry ], [ 0,
%next ]
On 2014/09/10 17:17:43, jvoung wrote:
> If you change the %arg to a recognizable constant, would that make it easier
to
> check for the assignment before the cmp?

Good idea, done.

https://codereview.chromium.org/560773002/diff/1/tests_lit/llvm2ice_tests/phi...
tests_lit/llvm2ice_tests/phi.ll:51: ; CHECK: jg
On 2014/09/10 17:17:43, jvoung wrote:
> Should this be CHECK-NEXT?

Done.

[jvoung (off chromium), 2014-09-10 18:39:42]

lgtm

[Jim Stichnoth, 2014-09-10 18:51:42]

Committed patchset #2 (id:20001) manually as e5a5be7 (presubmit successful).