Improve bool folding

Improve bool folding

Fold and/or followed by branch to eliminate cmp.  Also, fold fcmp instructions into branches similarly to what was done for icmp instructions.

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

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

[sehr, 2015-11-11 00:52:43]

Description was changed from

==========
Improve bool folding

Fold and/or followed by branch to eliminate cmp

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

to

==========
Improve bool folding

Fold and/or followed by branch to eliminate cmp.  Also, fold fcmp instructions
into branches similarly to what was done for icmp instructions.

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

[sehr, 2015-11-11 01:22:21]

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

[sehr, 2015-11-11 01:22:37]

Some small bool folding tweaks.

[John, 2015-11-11 02:08:10]

lgtm

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3026: _and(T, Src1);
would _test be better here? just curious.

[Jim Stichnoth, 2015-11-11 14:05:20]

Otherwise LGTM.

Strangely, the spec2k geomean was the same with and without this change, with a
2.5% gain in vpr balanced against a handful of 1% losses elsewhere.  Were you
expecting something different?

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:723: /// Emit the code for a combined compare and
branch, or sets the destination
Nit 1: "set" instead of "sets".
Nit 2: "if Br == nullptr" (for accuracy, and agreement with setccOrBr).
Nit 3: Document both methods with the same comment.

Suggestion:

  /// Emit the code for a combined icmp/branch or fcmp/branch, or set the
  /// destination variable of the compare if Br == nullptr.
  void lowerIcmpAndBr(const InstIcmp *Icmp, const InstBr *Br);
  void lowerFcmpAndBr(const InstIcmp *Icmp, const InstBr *Br);

Even better if you know how to make this combined comment doxygen-friendly.

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:135: break;
I think you should just "return PK_None", unless you expect to add the x86-32
i64 version in the future (in which case leave a TODO).

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2581: Operand *T = Src0;
std::swap(Src0, Src1);

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2683: } else {
Can this be "else if"?

  } else if (IntDefault == 0) {

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3026: _and(T, Src1);
On 2015/11/11 02:08:10, John wrote:
> would _test be better here? just curious.

Yes it would be better, since T would be able to share Src0's register.

https://codereview.chromium.org/1436623002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/fp.cmp.ll (right):

https://codereview.chromium.org/1436623002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fp.cmp.ll:4: ; there are no special OPTM1 match lines.
This intro is by now almost entirely untrue. :)

It never really was a comprehensive test because it only 6 of the 16 fcmp
conditions - the ones accessible in C.

And there now are OPTM1 lines (well, CHECK-OM1).

[sehr, 2015-11-11 18:57:03]

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

[sehr, 2015-11-13 06:00:52]

For the record.

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:723: /// Emit the code for a combined compare and
branch, or sets the destination
On 2015/11/11 14:05:20, stichnot wrote:
> Nit 1: "set" instead of "sets".
> Nit 2: "if Br == nullptr" (for accuracy, and agreement with setccOrBr).
> Nit 3: Document both methods with the same comment.
> 
> Suggestion:
> 
>   /// Emit the code for a combined icmp/branch or fcmp/branch, or set the
>   /// destination variable of the compare if Br == nullptr.
>   void lowerIcmpAndBr(const InstIcmp *Icmp, const InstBr *Br);
>   void lowerFcmpAndBr(const InstIcmp *Icmp, const InstBr *Br);
> 
> Even better if you know how to make this combined comment doxygen-friendly.

Doxygen is over my head :-).  Done otherwise.

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:135: break;
On 2015/11/11 14:05:20, stichnot wrote:
> I think you should just "return PK_None", unless you expect to add the x86-32
> i64 version in the future (in which case leave a TODO).

Done.

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2581: Operand *T = Src0;
On 2015/11/11 14:05:20, stichnot wrote:
> std::swap(Src0, Src1);

Done.

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2683: } else {
On 2015/11/11 14:05:20, stichnot wrote:
> Can this be "else if"?
> 
>   } else if (IntDefault == 0) {

I used std::swap and removed the duplication, as we discussed.

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3026: _and(T, Src1);
On 2015/11/11 02:08:10, John wrote:
> would _test be better here? just curious.

Done.

https://codereview.chromium.org/1436623002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3026: _and(T, Src1);
On 2015/11/11 14:05:20, stichnot wrote:
> On 2015/11/11 02:08:10, John wrote:
> > would _test be better here? just curious.
> 
> Yes it would be better, since T would be able to share Src0's register.

Done.

https://codereview.chromium.org/1436623002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/fp.cmp.ll (right):

https://codereview.chromium.org/1436623002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fp.cmp.ll:4: ; there are no special OPTM1 match lines.
On 2015/11/11 14:05:20, stichnot wrote:
> This intro is by now almost entirely untrue. :)
> 
> It never really was a comprehensive test because it only 6 of the 16 fcmp
> conditions - the ones accessible in C.
> 
> And there now are OPTM1 lines (well, CHECK-OM1).

I have removed the comment.