Subzero: Fix address mode optimization involving phi temporaries.

Subzero: Fix address mode optimization involving phi temporaries.

Also adds much-needed logging of the decision process that goes into the address mode optimization.

BUG= none
R=jvoung@chromium.org

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

[Jim Stichnoth, 2014-08-27 04:55:11]

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

[jvoung (off chromium), 2014-08-27 16:21:08]

https://codereview.chromium.org/490333003/diff/20001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/490333003/diff/20001/src/IceCfgNode.cpp#newco...
src/IceCfgNode.cpp:170: Dest->resetDefinition();
This is a way to indicate that the Dest variable is now multi-block?

https://codereview.chromium.org/490333003/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/490333003/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:3533: // TODO: ensure BaseVariable0 stays
single-BB
Does this take care of the TODO?

Maybe not the one below, because that's assuming the BaseInst is an add instead
of an assign.

https://codereview.chromium.org/490333003/diff/20001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/balloc.ll (right):

https://codereview.chromium.org/490333003/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/balloc.ll:8: ; RUN: %llvm2ice -O2 --verbose addropt %s
| FileCheck %s
It would probably be better to name the file after the bug instead of after the
function in which it occurred?

https://codereview.chromium.org/490333003/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/balloc.ll:66: %.asptr = inttoptr i32 %2 to i32*
Does the rest of this test matter, or can it just return %2, instead of
branching into the if.then8, if.else ?

[Jim Stichnoth, 2014-08-27 20:23:15]

https://codereview.chromium.org/490333003/diff/20001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/490333003/diff/20001/src/IceCfgNode.cpp#newco...
src/IceCfgNode.cpp:170: Dest->resetDefinition();
On 2014/08/27 16:21:07, jvoung wrote:
> This is a way to indicate that the Dest variable is now multi-block?

It's more about indicating Dest is multi-def, to prevent address mode
optimization from following definitions across phis.

With that said, I think it's probably better to make this more automatic inside
setDefinition(), so I reverted this resetDefinition() function.

https://codereview.chromium.org/490333003/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/490333003/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:3533: // TODO: ensure BaseVariable0 stays
single-BB
On 2014/08/27 16:21:07, jvoung wrote:
> Does this take care of the TODO?
> 
> Maybe not the one below, because that's assuming the BaseInst is an add
instead
> of an assign.

I don't think so.  This needs to be tuned, but my feeling about address mode
optimization is to try to avoid using variables in the expression that are
defined far away, because that may increase register pressure.  Single-BB is an
approximation for defined not far away.

https://codereview.chromium.org/490333003/diff/20001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/balloc.ll (right):

https://codereview.chromium.org/490333003/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/balloc.ll:8: ; RUN: %llvm2ice -O2 --verbose addropt %s
| FileCheck %s
On 2014/08/27 16:21:08, jvoung wrote:
> It would probably be better to name the file after the bug instead of after
the
> function in which it occurred?

Done.

https://codereview.chromium.org/490333003/diff/20001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/balloc.ll:66: %.asptr = inttoptr i32 %2 to i32*
On 2014/08/27 16:21:08, jvoung wrote:
> Does the rest of this test matter, or can it just return %2, instead of
> branching into the if.then8, if.else ?

Done.

[jvoung (off chromium), 2014-08-27 20:43:21]

LGTM

[Jim Stichnoth, 2014-08-27 20:50:07]

Committed patchset #3 manually as 89d7956 (presubmit successful).