[SubZero] Utilize instructions with immediate operands

[SubZero] Utilize instructions with immediate operands

This patch optimizes code generation of instructions with 16-bit immediate operands

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-11-04 11:41:15]

Description was changed from

==========
[SubZero] Utilize instructions with immediate operands

This patch optimizes code generation of instructions with 16-bit immediate
operands
==========

to

==========
[SubZero] Utilize instructions with immediate operands

This patch optimizes code generation of instructions with 16-bit immediate
operands
==========

[jaydeep.patil, 2016-11-04 11:41:15]

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

[Jim Stichnoth, 2016-11-04 13:18:48]

https://codereview.chromium.org/2478113003/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2478113003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2558: auto *Const32 =
llvm::dyn_cast<ConstantInteger32>(Src1);
Can you move the definition of Const32 into the switch statement where it is
used?  That makes it clearer that it is only used inside the switch statement.

https://codereview.chromium.org/2478113003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2585: if (Src1R == nullptr) {
Instead of the "(Src1R == nullptr)" idiom, I think it would be cleaner be more
explicit with a bool flag, e.g.

  bool IsSrc1Imm16 = false;
  ...
  if (Const32 != nullptr && ...) {
    IsSrc1Imm16 = true;
    Value = Const32->getValue();
  }
  ...
  if (IsSrc1Imm16) {

https://codereview.chromium.org/2478113003/diff/1/tests_lit/assembler/mips32/...
File tests_lit/assembler/mips32/encoding_test_arith.ll (right):

https://codereview.chromium.org/2478113003/diff/1/tests_lit/assembler/mips32/...
tests_lit/assembler/mips32/encoding_test_arith.ll:98: ; DIS-LABEL:00000020
<test_02>:
This is unfortunate - i.e., changes to an unrelated test affect this test's
output, just because of hard-coded code addresses.  Several other files in the
assembler/mips32/ directory may suffer from that in the future.

I see two ways to deal with this:

1. Just remove the code address offsets from the check strings, such as is done
in encoding_intrinsics.ll.

2. Add -ffunction-sections to the pnacl-sz args for all the RUN commands, or at
least those with --check-prefix=DIS.

It's OK to leave things as you have them for this CL, but you might want to
consider a separate cleanup CL to reduce future pain.

[jaydeep.patil, 2016-11-07 03:55:12]

https://codereview.chromium.org/2478113003/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2478113003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2558: auto *Const32 =
llvm::dyn_cast<ConstantInteger32>(Src1);
On 2016/11/04 13:18:48, Jim Stichnoth wrote:
> Can you move the definition of Const32 into the switch statement where it is
> used?  That makes it clearer that it is only used inside the switch statement.

Done.

https://codereview.chromium.org/2478113003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2585: if (Src1R == nullptr) {
On 2016/11/04 13:18:47, Jim Stichnoth wrote:
> Instead of the "(Src1R == nullptr)" idiom, I think it would be cleaner be more
> explicit with a bool flag, e.g.
> 
>   bool IsSrc1Imm16 = false;
>   ...
>   if (Const32 != nullptr && ...) {
>     IsSrc1Imm16 = true;
>     Value = Const32->getValue();
>   }
>   ...
>   if (IsSrc1Imm16) {

Done.

https://codereview.chromium.org/2478113003/diff/1/tests_lit/assembler/mips32/...
File tests_lit/assembler/mips32/encoding_test_arith.ll (right):

https://codereview.chromium.org/2478113003/diff/1/tests_lit/assembler/mips32/...
tests_lit/assembler/mips32/encoding_test_arith.ll:98: ; DIS-LABEL:00000020
<test_02>:
On 2016/11/04 13:18:48, Jim Stichnoth wrote:
> This is unfortunate - i.e., changes to an unrelated test affect this test's
> output, just because of hard-coded code addresses.  Several other files in the
> assembler/mips32/ directory may suffer from that in the future.
> 
> I see two ways to deal with this:
> 
> 1. Just remove the code address offsets from the check strings, such as is
done
> in encoding_intrinsics.ll.
> 
> 2. Add -ffunction-sections to the pnacl-sz args for all the RUN commands, or
at
> least those with --check-prefix=DIS.
> 
> It's OK to leave things as you have them for this CL, but you might want to
> consider a separate cleanup CL to reduce future pain.

I will submit a separate cleanup CL.

[Jim Stichnoth, 2016-11-07 04:53:40]

lgtm

[Jim Stichnoth, 2016-11-07 04:53:52]

Description was changed from

==========
[SubZero] Utilize instructions with immediate operands

This patch optimizes code generation of instructions with 16-bit immediate
operands
==========

to

==========
[SubZero] Utilize instructions with immediate operands

This patch optimizes code generation of instructions with 16-bit immediate
operands

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-11-07 04:53:53]

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