Calling convention for MIPS32

This patch provides calling convention class for MIPS with support of integer and floating point types

R=stichnot@chromium.org

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

[mohit.bhakkad, 2016-06-09 08:56:12]

Description was changed from

==========
Calling convention for MIPS32

BUG=
==========

to

==========
This patch provides calling convention class for MIPS with support of integer
and floating point types
==========

[mohit.bhakkad, 2016-06-09 08:56:12]

mohit.bhakkad@imgtec.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@google.com,
stichnot@chromium.org

[Jim Stichnoth, 2016-06-09 21:27:23]

https://codereview.chromium.org/2052793003/diff/1/src/IceRegistersMIPS32.h
File src/IceRegistersMIPS32.h (right):

https://codereview.chromium.org/2052793003/diff/1/src/IceRegistersMIPS32.h#ne...
src/IceRegistersMIPS32.h:93: // CAUTION: This is not entirely correct, as it
will return Reg_A2 even when
I think the caution should be stronger, such as something like
   assert(RegNum == Reg_A0A1 || RegNum == Reg_A2A3);
Otherwise you are just asking for trouble when some test case produces a
different register.

[mohit.bhakkad, 2016-06-10 12:54:35]

Modified according to MIPS O32 abi

[Jim Stichnoth, 2016-06-10 23:03:46]

https://codereview.chromium.org/2052793003/diff/40001/src/IceInstMIPS32.def
File src/IceInstMIPS32.def (right):

https://codereview.chromium.org/2052793003/diff/40001/src/IceInstMIPS32.def#n...
src/IceInstMIPS32.def:127: ALIASES2(Reg_F12, Reg_F12F13))                       
                       \
I would prepend 2 spaces so it lines up with Reg_F12 above.

https://codereview.chromium.org/2052793003/diff/40001/src/IceInstMIPS32.def#n...
src/IceInstMIPS32.def:128: X(Reg_F13,      13,  "f13",   1,0,0,0, 0,0,1,0,0,
ALIASES1(Reg_F13))         \
I think Reg_F12F13 should also be listed here.

In fact, all these registers should have their register pair listed as an alias.

The alias matrix should be symmetric, i.e. if i is an alias of j, then j should
be an alias of i.  I thought at least one other target validated this, but I
can't find any evidence of that in the current code...

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

https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:432: // TODO(mohit.bhakkad): Add support for
other types (struct as arg, functions
Fortunately, the PNaCl ABI only allows primitive (and vector) args and return
types.  Structs and other complexities are simplified upstream.

https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:441: UseFPRegs(false) {}
"UseFPRegs(false)" will be unnecessary when declaring "bool UseFPRegs = false;"
in the .h file.

https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:451: else if (isScalarFloatingType(Ty)) {
You don't need "else if" when the previous branch unconditionally returns.

https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:546: // In MIPS O32 abi if fun argumets are
(f32, i32) then one can not use reg_a0
arguments

https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:547: // for second argument even though its
free. f32 arg goes in reg_f12, i32 arg
it's

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

https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.h:495: // UseFPRegs is a flag indicationg if FP
registers can be used
indicating

https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.h:496: bool UseFPRegs;
I would use
  bool UseFPRegs = false;

[mohit.bhakkad, 2016-06-11 10:57:18]

On 2016/06/10 23:03:46, stichnot wrote:
> https://codereview.chromium.org/2052793003/diff/40001/src/IceInstMIPS32.def
> File src/IceInstMIPS32.def (right):
> 
>
https://codereview.chromium.org/2052793003/diff/40001/src/IceInstMIPS32.def#n...
> src/IceInstMIPS32.def:127: ALIASES2(Reg_F12, Reg_F12F13))                     
 
>                        \
> I would prepend 2 spaces so it lines up with Reg_F12 above.
> 
>
https://codereview.chromium.org/2052793003/diff/40001/src/IceInstMIPS32.def#n...
> src/IceInstMIPS32.def:128: X(Reg_F13,      13,  "f13",   1,0,0,0, 0,0,1,0,0,
> ALIASES1(Reg_F13))         \
> I think Reg_F12F13 should also be listed here.
> 
> In fact, all these registers should have their register pair listed as an
alias.
> 
> The alias matrix should be symmetric, i.e. if i is an alias of j, then j
should
> be an alias of i.  I thought at least one other target validated this, but I
> can't find any evidence of that in the current code...
> 
>
https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
> File src/IceTargetLoweringMIPS32.cpp (right):
> 
>
https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
> src/IceTargetLoweringMIPS32.cpp:432: // TODO(mohit.bhakkad): Add support for
> other types (struct as arg, functions
> Fortunately, the PNaCl ABI only allows primitive (and vector) args and return
> types.  Structs and other complexities are simplified upstream.
> 
>
https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
> src/IceTargetLoweringMIPS32.cpp:441: UseFPRegs(false) {}
> "UseFPRegs(false)" will be unnecessary when declaring "bool UseFPRegs =
false;"
> in the .h file.
> 
>
https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
> src/IceTargetLoweringMIPS32.cpp:451: else if (isScalarFloatingType(Ty)) {
> You don't need "else if" when the previous branch unconditionally returns.
> 
>
https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
> src/IceTargetLoweringMIPS32.cpp:546: // In MIPS O32 abi if fun argumets are
> (f32, i32) then one can not use reg_a0
> arguments
> 
>
https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
> src/IceTargetLoweringMIPS32.cpp:547: // for second argument even though its
> free. f32 arg goes in reg_f12, i32 arg
> it's
> 
>
https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
> File src/IceTargetLoweringMIPS32.h (right):
> 
>
https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
> src/IceTargetLoweringMIPS32.h:495: // UseFPRegs is a flag indicationg if FP
> registers can be used
> indicating
> 
>
https://codereview.chromium.org/2052793003/diff/40001/src/IceTargetLoweringMI...
> src/IceTargetLoweringMIPS32.h:496: bool UseFPRegs;
> I would use
>   bool UseFPRegs = false;

Thanks!!
Addressed review comments.

[Jim Stichnoth, 2016-06-11 14:13:26]

https://codereview.chromium.org/2052793003/diff/60001/src/IceInstMIPS32.def
File src/IceInstMIPS32.def (right):

https://codereview.chromium.org/2052793003/diff/60001/src/IceInstMIPS32.def#n...
src/IceInstMIPS32.def:114: X(Reg_F0,       0,   "f0",    1,0,0,0, 0,0,1,0,0,
ALIASES1(Reg_F0))          \
As mentioned before, for correctness, you also need to include the register-pair
registers in the aliases list, for all registers.

I.e., ALIASES2(Reg_F0, Reg_F0F1)

and do similar for all Reg_F* in this list.

Better to take care of this now rather than later.

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

https://codereview.chromium.org/2052793003/diff/60001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:451: } else {
More "else-after-return" simplifications can be done here.  I think the method
should probably look like this:

  if (isScalarIntegerType(Ty))
    return argInGPR(Ty, Reg);
  if (isScalarFloatingType(Ty)) {
    if (ArgNo == 0) {
      UseFPRegs = true;
      return argInVFP(Ty, Reg);
    }
    if (UseFPRegs && ArgNo == 1) {
      UseFPRegs = false;
      return argInVFP(Ty, Reg);
    }
    return argInGPR(Ty, Reg);
  }
  UnimplementedError(getFlags());
  return false;

[mohit.bhakkad, 2016-06-13 06:08:30]

- Added register-pairs for all the FP registers as alias.
- Did "else-after-return" simplifications as suggested.

[Jim Stichnoth, 2016-06-13 07:26:52]

lgtm

[Jim Stichnoth, 2016-06-13 07:28:17]

Description was changed from

==========
This patch provides calling convention class for MIPS with support of integer
and floating point types
==========

to

==========
This patch provides calling convention class for MIPS with support of integer
and floating point types

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-06-13 07:28:19]

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