Treat ORs as ADDs for address optimization if operand has enough zero bits on the right

Treat ORs as ADDs for address optimization if operand has enough zero bits on the right

  Index=Var Or Const to Index=Var + Const
  when Var = Var' << N and log2(Const) <= N
  or when Var = (2^M) * (2^N) and log2(Const) <= (M+N)
BUG=none
R=stichnot@chromium.org

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

[manasijm, 2016-06-22 18:54:10]

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

[Jim Stichnoth, 2016-06-22 20:01:14]

Some initial comments.  I still want to look deeper at the implementation and
the generated code.

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5288: ArithInst->getOp() !=
InstArithmetic::Or)
Once we're up to testing 3 values, I think this should be turned into a switch
statement.

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5318: auto uintLog2 = [](uint32_t x) {
Can you just use llvm::Log2_32() or similar?

See src/llvm/include/llvm/Support/MathExtras.h

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5324: SizeT numZeroes = 0;
NumZeroes

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5326: if (auto ConstInt =
auto *

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5331: ConstantInteger32 *MultConst = nullptr;
Combine this and the next line.

https://codereview.chromium.org/2085383002/diff/1/tests_lit/llvm2ice_tests/ad...
File tests_lit/llvm2ice_tests/address-mode-opt.ll (right):

https://codereview.chromium.org/2085383002/diff/1/tests_lit/llvm2ice_tests/ad...
tests_lit/llvm2ice_tests/address-mode-opt.ll:156: %addr.int = or i32 1, %shl1
I would like to see two more tests:

(1) "or i32 4, %shl1"
Make sure there's no off-by-one error that incorrectly triggers the
optimization.

(2) "or i32 -1, %shl1"
Make sure there's no signed/unsigned error that incorrectly triggers the
optimization.

https://codereview.chromium.org/2085383002/diff/1/tests_lit/llvm2ice_tests/ad...
tests_lit/llvm2ice_tests/address-mode-opt.ll:161: ; CHECK: movss xmm0,DWORD PTR
[eax*4+0x1]
It would be better to not hard-code specific registers.  E.g.:

  CHECK: movss xmm{{[0-9]+}},DWORD PTR [{{e..}}*4+0x1]

[manasijm, 2016-06-27 18:04:19]

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5288: ArithInst->getOp() !=
InstArithmetic::Or)
On 2016/06/22 20:01:14, stichnot wrote:
> Once we're up to testing 3 values, I think this should be turned into a switch
> statement.

Done.

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5318: auto uintLog2 = [](uint32_t x) {
On 2016/06/22 20:01:13, stichnot wrote:
> Can you just use llvm::Log2_32() or similar?
> 
> See src/llvm/include/llvm/Support/MathExtras.h

Done.

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5324: SizeT numZeroes = 0;
On 2016/06/22 20:01:13, stichnot wrote:
> NumZeroes

Done.

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5326: if (auto ConstInt =
On 2016/06/22 20:01:13, stichnot wrote:
> auto *

Done.

https://codereview.chromium.org/2085383002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5331: ConstantInteger32 *MultConst = nullptr;
On 2016/06/22 20:01:13, stichnot wrote:
> Combine this and the next line.

Done.

https://codereview.chromium.org/2085383002/diff/1/tests_lit/llvm2ice_tests/ad...
File tests_lit/llvm2ice_tests/address-mode-opt.ll (right):

https://codereview.chromium.org/2085383002/diff/1/tests_lit/llvm2ice_tests/ad...
tests_lit/llvm2ice_tests/address-mode-opt.ll:156: %addr.int = or i32 1, %shl1
On 2016/06/22 20:01:14, stichnot wrote:
> I would like to see two more tests:
> 
> (1) "or i32 4, %shl1"
> Make sure there's no off-by-one error that incorrectly triggers the
> optimization.
> 
> (2) "or i32 -1, %shl1"
> Make sure there's no signed/unsigned error that incorrectly triggers the
> optimization.

Done.

https://codereview.chromium.org/2085383002/diff/1/tests_lit/llvm2ice_tests/ad...
tests_lit/llvm2ice_tests/address-mode-opt.ll:161: ; CHECK: movss xmm0,DWORD PTR
[eax*4+0x1]
On 2016/06/22 20:01:14, stichnot wrote:
> It would be better to not hard-code specific registers.  E.g.:
> 
>   CHECK: movss xmm{{[0-9]+}},DWORD PTR [{{e..}}*4+0x1]

Done.

[Jim Stichnoth, 2016-06-27 20:33:05]

otherwise LGTM

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

https://codereview.chromium.org/2085383002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:5315: return nullptr;
Use braces around the return statement for consistency.  Generally, I'm OK with:

  if (C)
    A;
  else
    B;

or

  if (C) {
    A;
  } else {
    B;
  }

but not a mismatch of the two styles.

[manasijm, 2016-06-27 20:47:42]

Description was changed from

==========
Treat ORs as ADDs for address optimization if operand has enough zero bits on
the right

  Index=Var Or Const to Index=Var + Const
  when Var = Var' << N and log2(Const) <= N
  or when Var = (2^M) * (2^N) and log2(Const) <= (M+N)
BUG=none
==========

to

==========
Treat ORs as ADDs for address optimization if operand has enough zero bits on
the right

  Index=Var Or Const to Index=Var + Const
  when Var = Var' << N and log2(Const) <= N
  or when Var = (2^M) * (2^N) and log2(Const) <= (M+N)
BUG=none
R=stichnot@chromium.org

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

[manasijm, 2016-06-27 20:47:43]

Committed patchset #3 (id:40001) manually as
9dd397d322848cbc9c54e80acb6dc0a2b30aaeb7 (presubmit successful).