[SubZero] Use DIV instruction instead of TargetHelperCall

[SubZero] Use DIV instruction instead of TargetHelperCall

Use DIV/DIVU instructions provided by MIPS32 ISA instead of calling target
helper function (__divsi3 etc.). These instructions give 32-bit quotient and
remainder in 32-bit special LO/HI registers respectively. An additional
instructions to check for divide-by-zero (Trap if equal) is emitted after the
DIV/DIVU instructions.

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-09-16 05:52:48]

Description was changed from

==========
[SubZero] Use DIV instruction instead of TargetHelperCall

Use DIV/DIVU instructions provided by MIPS32 ISA instead of calling target
helper function (__divsi3 etc.). These instructions give 32-bit quotient and
remainder in 32-bit special LO/HI registers respectively. An additional
instructions to check for divide-by-zero (Trap if equal) is emitted after the
DIV/DIVU instructions.
==========

to

==========
[SubZero] Use DIV instruction instead of TargetHelperCall

Use DIV/DIVU instructions provided by MIPS32 ISA instead of calling target
helper function (__divsi3 etc.). These instructions give 32-bit quotient and
remainder in 32-bit special LO/HI registers respectively. An additional
instructions to check for divide-by-zero (Trap if equal) is emitted after the
DIV/DIVU instructions.
==========

[jaydeep.patil, 2016-09-16 05:52:48]

jaydeep.patil@imgtec.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@google.com,
sagar.thakur@imgtec.com, srdjan.obucina@imgtec.com, stichnot@chromium.org

[obucinac, 2016-09-16 10:30:41]

obucinac@gmail.com changed reviewers:
+ obucinac@gmail.com

[obucinac, 2016-09-16 10:30:42]

https://codereview.chromium.org/2339323004/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2339323004/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:92: template <> const char *InstMIPS32Teq::Opcode = "teq";
Alphabetize

https://codereview.chromium.org/2339323004/diff/1/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2339323004/diff/1/src/IceInstMIPS32.h#newcode...
src/IceInstMIPS32.h:1185: using InstMIPS32Teq = InstMIPS32Trap<InstMIPS32::Teq>;
Alphabetize

https://codereview.chromium.org/2339323004/diff/1/src/IceTargetLoweringMIPS32.h
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/2339323004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:293: 
Alphabetize

[jaydeep.patil, 2016-09-16 10:39:07]

https://codereview.chromium.org/2339323004/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2339323004/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:92: template <> const char *InstMIPS32Teq::Opcode = "teq";
On 2016/09/16 10:30:42, obucinac wrote:
> Alphabetize

Done.

https://codereview.chromium.org/2339323004/diff/1/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2339323004/diff/1/src/IceInstMIPS32.h#newcode...
src/IceInstMIPS32.h:1185: using InstMIPS32Teq = InstMIPS32Trap<InstMIPS32::Teq>;
On 2016/09/16 10:30:42, obucinac wrote:
> Alphabetize

Done.

https://codereview.chromium.org/2339323004/diff/1/src/IceTargetLoweringMIPS32.h
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/2339323004/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:293: 
On 2016/09/16 10:30:42, obucinac wrote:
> Alphabetize

Done.

[Jim Stichnoth, 2016-09-16 16:33:02]

https://codereview.chromium.org/2339323004/diff/20001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2339323004/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:926: const uint32_t Tcode) {
I would remove the "const" here.

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1895: _teq(Src1R, T_Zero, 7); // Trap if
divide-by-zero
I like to avoid magic constants when possible.

Instead, maybe somewhere before this switch statement:
  constexpr uint32_t TrapIfDivideByZero = 7;

or whatever it should be named to convey the proper semantics (I'm just guessing
here).

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.h:508: void _teq(Variable *Src0, Variable *Src1,
const uint32_t TrapCode) {
I would remove the "const" here.

[jaydeep.patil, 2016-09-19 03:30:51]

https://codereview.chromium.org/2339323004/diff/20001/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2339323004/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:926: const uint32_t Tcode) {
On 2016/09/16 16:33:02, stichnot wrote:
> I would remove the "const" here.

Done.

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1895: _teq(Src1R, T_Zero, 7); // Trap if
divide-by-zero
On 2016/09/16 16:33:02, stichnot wrote:
> I like to avoid magic constants when possible.
> 
> Instead, maybe somewhere before this switch statement:
>   constexpr uint32_t TrapIfDivideByZero = 7;
> 
> or whatever it should be named to convey the proper semantics (I'm just
guessing
> here).

Done.

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.h:508: void _teq(Variable *Src0, Variable *Src1,
const uint32_t TrapCode) {
On 2016/09/16 16:33:02, stichnot wrote:
> I would remove the "const" here.

Done.

[obucinac, 2016-09-19 03:37:04]

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1903: _teq(Src1R, T_Zero, 7); // Trap if
divide-by-zero
auto *T_Zero = I32Reg(RegMIPS32::Reg_ZERO);
_teq(Src1R, T_Zero, 7);

May be changed with simpler

_teq(Src1R, getZero(), 7);

[jaydeep.patil, 2016-09-19 05:04:40]

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2339323004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1903: _teq(Src1R, T_Zero, 7); // Trap if
divide-by-zero
On 2016/09/19 03:37:04, obucinac wrote:
> auto *T_Zero = I32Reg(RegMIPS32::Reg_ZERO);
> _teq(Src1R, T_Zero, 7);
> 
> May be changed with simpler
> 
> _teq(Src1R, getZero(), 7);

Calling getZero() for each occurrence of T_Zero is not efficient.

[Jim Stichnoth, 2016-09-19 23:48:24]

lgtm

[Jim Stichnoth, 2016-09-19 23:48:37]

Description was changed from

==========
[SubZero] Use DIV instruction instead of TargetHelperCall

Use DIV/DIVU instructions provided by MIPS32 ISA instead of calling target
helper function (__divsi3 etc.). These instructions give 32-bit quotient and
remainder in 32-bit special LO/HI registers respectively. An additional
instructions to check for divide-by-zero (Trap if equal) is emitted after the
DIV/DIVU instructions.
==========

to

==========
[SubZero] Use DIV instruction instead of TargetHelperCall

Use DIV/DIVU instructions provided by MIPS32 ISA instead of calling target
helper function (__divsi3 etc.). These instructions give 32-bit quotient and
remainder in 32-bit special LO/HI registers respectively. An additional
instructions to check for divide-by-zero (Trap if equal) is emitted after the
DIV/DIVU instructions.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-09-19 23:48:38]

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