Subzero. ARM32. Strength reduce multiplications.

Subzero. ARM32. Strength reduce multiplications.

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

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

[John, 2015-11-24 15:15:01]

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

[Jim Stichnoth, 2015-11-24 19:32:20]

sick lgtm bro

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2051: // or two shifts to materialize it. A
sequence of 1s, i.e.
e.g. instead of i.e., right?

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2108: uint32_t ShAmt = 33;
I assumed 33 is meant to be an invalid value when using the default zero-arg
ctor?  Can you add a comment like "invalid shift amount"?

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2112: bool addOperations(uint32_t RangeStart,
uint32_t RangeEnd, SizeT *NumOperations,
I think it might be helpful to document that [RangeStart, RangeEnd] is a closed
interval.

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2153: //   * clears all the upper bits in NegSrc
up bit L;
up to bit

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2158: // of 1s starting at L, and ending a nL +
1, was found.
ending at

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2382: const bool OptM1 =
Ctx->getFlags().getOptLevel() == Opt_m1;
In IceTargetLoweringX86BaseImpl.h, strength-reduced multiplication is done for
Opt_1 and above (i.e. not Opt_0).  This was kind of an arbitrary decision and
should be revisited, but it would be good to have a consistent policy across
targets.

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2390: Operand *_0 =
legalize(Ctx->getConstantInt32(0), Legal_Reg | Legal_Flex);
Instead of getConstantInt32(0), it may be better to use
getConstantZero(Dest->getType()), in case we care down the road about type
consistency in the operands.

https://codereview.chromium.org/1469113003/diff/100001/crosstest/test_arith.def
File crosstest/test_arith.def (right):

https://codereview.chromium.org/1469113003/diff/100001/crosstest/test_arith.d...
crosstest/test_arith.def:84: #define MULIMM_TABLE   \
80-col alignment.  Tag, you're it!

[John, 2015-11-24 20:26:55]

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2051: // or two shifts to materialize it. A
sequence of 1s, i.e.
On 2015/11/24 19:32:20, stichnot wrote:
> e.g. instead of i.e., right?

Done.

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2108: uint32_t ShAmt = 33;
On 2015/11/24 19:32:20, stichnot wrote:
> I assumed 33 is meant to be an invalid value when using the default zero-arg
> ctor?  Can you add a comment like "invalid shift amount"?

This used to matter when this element was a const. removed.

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2112: bool addOperations(uint32_t RangeStart,
uint32_t RangeEnd, SizeT *NumOperations,
On 2015/11/24 19:32:20, stichnot wrote:
> I think it might be helpful to document that [RangeStart, RangeEnd] is a
closed
> interval.

Done.

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2153: //   * clears all the upper bits in NegSrc
up bit L;
On 2015/11/24 19:32:20, stichnot wrote:
> up to bit

Done.

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2158: // of 1s starting at L, and ending a nL +
1, was found.
On 2015/11/24 19:32:20, stichnot wrote:
> ending at

Done.

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2382: const bool OptM1 =
Ctx->getFlags().getOptLevel() == Opt_m1;
On 2015/11/24 19:32:20, stichnot wrote:
> In IceTargetLoweringX86BaseImpl.h, strength-reduced multiplication is done for
> Opt_1 and above (i.e. not Opt_0).  This was kind of an arbitrary decision and
> should be revisited, but it would be good to have a consistent policy across
> targets.

In this case, I ran ammp with 4 and 5 shifts, and 5 was slightly/marginally
worse. 4 allows multiplications by constants that have two "ranges" of 1s.

https://codereview.chromium.org/1469113003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2390: Operand *_0 =
legalize(Ctx->getConstantInt32(0), Legal_Reg | Legal_Flex);
On 2015/11/24 19:32:20, stichnot wrote:
> Instead of getConstantInt32(0), it may be better to use
> getConstantZero(Dest->getType()), in case we care down the road about type
> consistency in the operands.

Done.

https://codereview.chromium.org/1469113003/diff/100001/crosstest/test_arith.def
File crosstest/test_arith.def (right):

https://codereview.chromium.org/1469113003/diff/100001/crosstest/test_arith.d...
crosstest/test_arith.def:84: #define MULIMM_TABLE   \
On 2015/11/24 19:32:20, stichnot wrote:
> 80-col alignment.  Tag, you're it!

Done.

[John, 2015-11-24 20:30:05]

Committed patchset #7 (id:120001) manually as
98cc08ca9749fbdd0142ea3152616cd5476d727d (presubmit successful).