Optimize 64-bit compares with zero

Optimize 64-bit compares with zero

Comparisons with zero can be done with no branches in most cases and with
simpler sequences of operations.

BUG=
R=stichnot@chromium.org

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

[sehr, 2015-10-14 16:55:43]

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

[sehr, 2015-10-14 16:56:00]

Thanks in advance.

[Jim Stichnoth, 2015-10-14 18:04:49]

otherwise lgtm

https://chromiumcodereview.appspot.com/1406593003/diff/40001/crosstest/test_i...
File crosstest/test_icmp.cpp (right):

https://chromiumcodereview.appspot.com/1406593003/diff/40001/crosstest/test_i...
crosstest/test_icmp.cpp:32: bool icmp_zero##cmp(uint8_t a) { return a op 0; }   
                        \
Can you just add these to the above list, like it's done in the .h file?

https://chromiumcodereview.appspot.com/1406593003/diff/40001/src/IceTargetLow...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://chromiumcodereview.appspot.com/1406593003/diff/40001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:2812: case InstIcmp::Ugt:
I think you should combine Ugt with Ne, and Ule with Eq, because really, not by
coincidence, they are in fact equivalent.

[sehr, 2015-10-15 00:19:34]

Thanks for the review and the help getting this going.  PTAL.

https://chromiumcodereview.appspot.com/1406593003/diff/40001/crosstest/test_i...
File crosstest/test_icmp.cpp (right):

https://chromiumcodereview.appspot.com/1406593003/diff/40001/crosstest/test_i...
crosstest/test_icmp.cpp:32: bool icmp_zero##cmp(uint8_t a) { return a op 0; }   
                        \
On 2015/10/14 18:04:49, stichnot wrote:
> Can you just add these to the above list, like it's done in the .h file?

Done.

https://chromiumcodereview.appspot.com/1406593003/diff/40001/src/IceTargetLow...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://chromiumcodereview.appspot.com/1406593003/diff/40001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:2812: case InstIcmp::Ugt:
On 2015/10/14 18:04:49, stichnot wrote:
> I think you should combine Ugt with Ne, and Ule with Eq, because really, not
by
> coincidence, they are in fact equivalent.

Good point.  Done.

[Jim Stichnoth, 2015-10-15 00:48:48]

still lgtm

https://chromiumcodereview.appspot.com/1406593003/diff/60001/src/IceTargetLow...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://chromiumcodereview.appspot.com/1406593003/diff/60001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:2775: (Ty == IceType_i32 || Ty == IceType_i16
|| Ty == IceType_i8)) {
Do you need these tests on Ty?

Vector and FP types are already covered above.

It seems like it should work just fine for I64, except the dyn_cast won't
trigger on ConstantInteger64.

I think I1 should be fine as well.

https://chromiumcodereview.appspot.com/1406593003/diff/60001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:2812: if (llvm::isa<ConstantInteger64>(Src1)
&&
I don't like isa<> followed by cast<> or dyn_cast<>.

How about a helper in an anonymous namespace:

bool isZero(const Operand *Opnd) {
  if (auto *C64 = llvm::dyn_cast<ConstantInteger64)(Opnd))
    return C64->getValue() == 0;
  if (auto *C32 = llvm::dyn_cast<ConstantInteger32)(Opnd))
    return C32->getValue() == 0;
  return false;
}

That helper could probably also be used above.

https://chromiumcodereview.appspot.com/1406593003/diff/60001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:2822: // These four or the high and low half
and compare, so they need the
s/or/are/ ?

[sehr, 2015-10-15 17:33:46]

Thanks again.  Waiting for spec to finish.

https://chromiumcodereview.appspot.com/1406593003/diff/60001/src/IceTargetLow...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://chromiumcodereview.appspot.com/1406593003/diff/60001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:2775: (Ty == IceType_i32 || Ty == IceType_i16
|| Ty == IceType_i8)) {
On 2015/10/15 00:48:47, stichnot wrote:
> Do you need these tests on Ty?
> 
> Vector and FP types are already covered above.
> 
> It seems like it should work just fine for I64, except the dyn_cast won't
> trigger on ConstantInteger64.
> 
> I think I1 should be fine as well.

Removed.

https://chromiumcodereview.appspot.com/1406593003/diff/60001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:2812: if (llvm::isa<ConstantInteger64>(Src1)
&&
On 2015/10/15 00:48:47, stichnot wrote:
> I don't like isa<> followed by cast<> or dyn_cast<>.
> 
> How about a helper in an anonymous namespace:
> 
> bool isZero(const Operand *Opnd) {
>   if (auto *C64 = llvm::dyn_cast<ConstantInteger64)(Opnd))
>     return C64->getValue() == 0;
>   if (auto *C32 = llvm::dyn_cast<ConstantInteger32)(Opnd))
>     return C32->getValue() == 0;
>   return false;
> }
> 
> That helper could probably also be used above.

Done.

https://chromiumcodereview.appspot.com/1406593003/diff/60001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:2822: // These four or the high and low half
and compare, so they need the
On 2015/10/15 00:48:47, stichnot wrote:
> s/or/are/ ?

Reworded to avoid the convoluted mis-parse possible here.

[sehr, 2015-10-15 17:38:58]

Committed patchset #5 (id:80001) manually as
5c87542ab707c91b1cc0d3d32ff4073e27f9bdf7 (presubmit successful).