Replace constant conditional branches by unconditional branches

Replace constant conditional branches by unconditional branches

BUG=
R=jpp@chromium.org, stichnot@chromium.org

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

[sehr, 2016-04-01 23:31:58]

Description was changed from

==========
Replace constant conditional branches by unconditional branches

BUG=
==========

to

==========
Replace constant conditional branches by unconditional branches

BUG=
==========

[sehr, 2016-04-01 23:31:58]

sehr@chromium.org changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@chromium.org,
stichnot@chromium.org

[sehr, 2016-04-01 23:33:31]

Trying the build now.  It definitely removes a bunch of code in the pathological
pexe very early on.  Bonus points would be for us to now merge blocks:

B1:
  ...
  br B2
B2:
  ...

is all over the code after my change.

[John, 2016-04-02 01:02:55]

lgtm

[Jim Stichnoth, 2016-04-02 01:19:52]

lgtm

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

https://codereview.chromium.org/1847423003/diff/1/src/IceCfgNode.cpp#newcode90
src/IceCfgNode.cpp:90: // that phi arguments with the same label have the same
value.
Update this comment to mention the cleanup now being done.

https://codereview.chromium.org/1847423003/diff/1/src/IceCfgNode.cpp#newcode91
src/IceCfgNode.cpp:91: void CfgNode::validatePhis() {
I'm thinking that maybe this method name should be changed because it's no
longer a read-only validation.

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

https://codereview.chromium.org/1847423003/diff/1/src/IceInst.cpp#newcode307
src/IceInst.cpp:307: 
remove initial blank line

https://codereview.chromium.org/1847423003/diff/1/src/IceInst.cpp#newcode308
src/IceInst.cpp:308: if (auto *Constant =
llvm::dyn_cast<ConstantInteger32>(Source)) {
I would name it something like "C32", that isn't an ICE class name.

https://codereview.chromium.org/1847423003/diff/1/src/IceInst.cpp#newcode414
src/IceInst.cpp:414: // node by a zero of the appropriate type. TODO: This uses
a linear-time search,
TODO(owner)

Actually, maybe this TODO and the identical one above should just be removed,
because it sort of obscures other TODOs that actually need doing.  I.e., I have
no intention of improving the linear-time complexity until it actually shows up
as a problem, and if it does, it should be readily apparent via profiling.

[sehr, 2016-04-04 15:38:52]

Thanks for the reviews.  Waiting for the llvm builds (thanks, Karl!) and will
re-run presubmit.

David

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

https://codereview.chromium.org/1847423003/diff/1/src/IceCfgNode.cpp#newcode90
src/IceCfgNode.cpp:90: // that phi arguments with the same label have the same
value.
On 2016/04/02 01:19:52, stichnot wrote:
> Update this comment to mention the cleanup now being done.

Done.

https://codereview.chromium.org/1847423003/diff/1/src/IceCfgNode.cpp#newcode91
src/IceCfgNode.cpp:91: void CfgNode::validatePhis() {
On 2016/04/02 01:19:52, stichnot wrote:
> I'm thinking that maybe this method name should be changed because it's no
> longer a read-only validation.

I changed it to enforcePhiConsistency.  WDYT?

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

https://codereview.chromium.org/1847423003/diff/1/src/IceInst.cpp#newcode307
src/IceInst.cpp:307: 
On 2016/04/02 01:19:52, stichnot wrote:
> remove initial blank line

Done.

https://codereview.chromium.org/1847423003/diff/1/src/IceInst.cpp#newcode308
src/IceInst.cpp:308: if (auto *Constant =
llvm::dyn_cast<ConstantInteger32>(Source)) {
On 2016/04/02 01:19:52, stichnot wrote:
> I would name it something like "C32", that isn't an ICE class name.

Done.

https://codereview.chromium.org/1847423003/diff/1/src/IceInst.cpp#newcode414
src/IceInst.cpp:414: // node by a zero of the appropriate type. TODO: This uses
a linear-time search,
On 2016/04/02 01:19:52, stichnot wrote:
> TODO(owner)
> 
> Actually, maybe this TODO and the identical one above should just be removed,
> because it sort of obscures other TODOs that actually need doing.  I.e., I
have
> no intention of improving the linear-time complexity until it actually shows
up
> as a problem, and if it does, it should be readily apparent via profiling.

Done.

[Jim Stichnoth, 2016-04-04 15:50:31]

still lgtm.

BTW this seemed to give a 10% translation time improvement on the pexe in
question, with 25% improvement on the ginormous function.

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

https://codereview.chromium.org/1847423003/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:90: // value must be zero.  The latter allows us to remove
some dead control flow
I suggest this change of the "For every phi argument..." sentence for clarity:

If a phi argument's label doesn't appear in the predecessor list (which can
happen as a result of e.g. unreachable node elimination), its value is modified
to be zero, to maintain consistency in liveness analysis.

https://codereview.chromium.org/1847423003/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:91: // without a major rework of the phi nodes.  We don't
check that phi arguments
I prefer "phi instruction" over "phi node", but your call...

[sehr, 2016-04-04 17:06:41]

Thanks again.

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

https://codereview.chromium.org/1847423003/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:90: // value must be zero.  The latter allows us to remove
some dead control flow
On 2016/04/04 15:50:31, stichnot wrote:
> I suggest this change of the "For every phi argument..." sentence for clarity:
> 
> If a phi argument's label doesn't appear in the predecessor list (which can
> happen as a result of e.g. unreachable node elimination), its value is
modified
> to be zero, to maintain consistency in liveness analysis.

Done.

https://codereview.chromium.org/1847423003/diff/20001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:91: // without a major rework of the phi nodes.  We don't
check that phi arguments
On 2016/04/04 15:50:31, stichnot wrote:
> I prefer "phi instruction" over "phi node", but your call...

Done.

[sehr, 2016-04-04 17:11:11]

Description was changed from

==========
Replace constant conditional branches by unconditional branches

BUG=
==========

to

==========
Replace constant conditional branches by unconditional branches

BUG=
R=jpp@chromium.org, stichnot@chromium.org

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

[sehr, 2016-04-04 17:11:12]

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