Fixes bug on conditional branch where the targets are the same.

Fixes bug on conditional branch where the targets are the same.

Fixes constructor InstBr when it is a conditional branch, and the
true and false branches are the same.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4212
R=jpp@chromium.org, stichnot@chromium.org

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

[Karl, 2015-06-25 20:33:30]

kschimpf@google.com changed reviewers:
+ jpp@chromium.org, jvoung@chromium.org, stichnot@chromium.org

[John, 2015-06-25 21:15:35]

lgtm

oh, the beauty of LLVM coding style...

https://codereview.chromium.org/1215443002/diff/1/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1215443002/diff/1/src/IceInst.cpp#newcode275
src/IceInst.cpp:275: TargetTrue(TargetTrue_) {
the trailing _ is a great way to differentiate member variables from parameters,
but this is not done (AFAIK) anywhere else.

I would
1) use a different naming convention (ok, this may be hard :)); or
2) change l277 to this->TargetTrue = nullptr

[Jim Stichnoth, 2015-06-25 21:26:30]

https://codereview.chromium.org/1215443002/diff/1/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1215443002/diff/1/src/IceInst.cpp#newcode275
src/IceInst.cpp:275: TargetTrue(TargetTrue_) {
On 2015/06/25 21:15:35, John wrote:
> the trailing _ is a great way to differentiate member variables from
parameters,
> but this is not done (AFAIK) anywhere else.
> 
> I would
> 1) use a different naming convention (ok, this may be hard :)); or
> 2) change l277 to this->TargetTrue = nullptr

I'm very sad that clang didn't give any warnings about this.

I plan to do a CL to add these trailing underscores where appropriate, and also
turn on a warning if it exists.

https://codereview.chromium.org/1215443002/diff/1/tests_lit/llvm2ice_tests/co...
File tests_lit/llvm2ice_tests/cond-br-same-target.ll (right):

https://codereview.chromium.org/1215443002/diff/1/tests_lit/llvm2ice_tests/co...
tests_lit/llvm2ice_tests/cond-br-same-target.ll:4: ; RUN: %p2i -i %s --args -O2
--verbose inst -threads=0 | FileCheck %s
You should be able to use "--insts" instead of "--verbose inst -threads=0".  (It
also adds -notranslate, which is actually fine given comments below.)

Also, it would be nice to add an Om1 run, to demonstrate that this is a
fundamental transformation that doesn't depend on the optimization level.

BTW, it occurs to me that doing such a transformation in a high-level ICE
instruction's ctor, is kind of not kosher, because we mean for a Cfg::dump()
called on a freshly parsed function to output pretty much the same as the .ll
file input.  But in this example, we dump an unconditional branch for a
conditional branch input.  Oh well.

https://codereview.chromium.org/1215443002/diff/1/tests_lit/llvm2ice_tests/co...
tests_lit/llvm2ice_tests/cond-br-same-target.ll:13: ; The following shows that
the input is parsed correctly.
Actually, it shows that conditional has already become unconditional.

https://codereview.chromium.org/1215443002/diff/1/tests_lit/llvm2ice_tests/co...
tests_lit/llvm2ice_tests/cond-br-same-target.ll:15: ; CHECK:     
================ Initial CFG ================
Probably better not to check for this header, which I don't consider part of the
"stable" dump output.

https://codereview.chromium.org/1215443002/diff/1/tests_lit/llvm2ice_tests/co...
tests_lit/llvm2ice_tests/cond-br-same-target.ll:24: ; The following shows the
conditional branch is converted to unconditional.
Given my earlier comment, I don't think this latter check is needed.  In fact,
the very act of dumping the initial CFG is enough to trigger the assertion
failure.

[Karl, 2015-06-25 22:33:42]

https://codereview.chromium.org/1215443002/diff/1/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1215443002/diff/1/src/IceInst.cpp#newcode275
src/IceInst.cpp:275: TargetTrue(TargetTrue_) {
On 2015/06/25 21:26:29, stichnot wrote:
> On 2015/06/25 21:15:35, John wrote:
> > the trailing _ is a great way to differentiate member variables from
> parameters,
> > but this is not done (AFAIK) anywhere else.
> > 
> > I would
> > 1) use a different naming convention (ok, this may be hard :)); or
> > 2) change l277 to this->TargetTrue = nullptr
> 
> I'm very sad that clang didn't give any warnings about this.
> 
> I plan to do a CL to add these trailing underscores where appropriate, and
also
> turn on a warning if it exists.

Leaving as is, assuming that Jim will follow through and fix other instances.

https://codereview.chromium.org/1215443002/diff/1/tests_lit/llvm2ice_tests/co...
File tests_lit/llvm2ice_tests/cond-br-same-target.ll (right):

https://codereview.chromium.org/1215443002/diff/1/tests_lit/llvm2ice_tests/co...
tests_lit/llvm2ice_tests/cond-br-same-target.ll:4: ; RUN: %p2i -i %s --args -O2
--verbose inst -threads=0 | FileCheck %s
On 2015/06/25 21:26:29, stichnot wrote:
> You should be able to use "--insts" instead of "--verbose inst -threads=0". 
(It
> also adds -notranslate, which is actually fine given comments below.)
> 
> Also, it would be nice to add an Om1 run, to demonstrate that this is a
> fundamental transformation that doesn't depend on the optimization level.
> 
> BTW, it occurs to me that doing such a transformation in a high-level ICE
> instruction's ctor, is kind of not kosher, because we mean for a Cfg::dump()
> called on a freshly parsed function to output pretty much the same as the .ll
> file input.  But in this example, we dump an unconditional branch for a
> conditional branch input.  Oh well.

Done.

https://codereview.chromium.org/1215443002/diff/1/tests_lit/llvm2ice_tests/co...
tests_lit/llvm2ice_tests/cond-br-same-target.ll:13: ; The following shows that
the input is parsed correctly.
On 2015/06/25 21:26:29, stichnot wrote:
> Actually, it shows that conditional has already become unconditional.

Correct. The test now shows it.

https://codereview.chromium.org/1215443002/diff/1/tests_lit/llvm2ice_tests/co...
tests_lit/llvm2ice_tests/cond-br-same-target.ll:15: ; CHECK:     
================ Initial CFG ================
On 2015/06/25 21:26:29, stichnot wrote:
> Probably better not to check for this header, which I don't consider part of
the
> "stable" dump output.

Removed.

https://codereview.chromium.org/1215443002/diff/1/tests_lit/llvm2ice_tests/co...
tests_lit/llvm2ice_tests/cond-br-same-target.ll:24: ; The following shows the
conditional branch is converted to unconditional.
On 2015/06/25 21:26:30, stichnot wrote:
> Given my earlier comment, I don't think this latter check is needed.  In fact,
> the very act of dumping the initial CFG is enough to trigger the assertion
> failure.

Removed.

[Jim Stichnoth, 2015-06-26 00:52:04]

lgtm

[Karl, 2015-06-26 17:25:37]

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