Subzero, MIPS32: Introduction of floating point registers

Subzero, MIPS32: Introduction of floating point registers

This patch introduces floating point registers used for 32-bit
operations, and basic handling of FP values in operands. It is
partial work needed as a base for further work.

R=stichnot@chromium.org

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

[obucinac, 2016-05-19 15:14:54]

Description was changed from

==========
Subzero, MIPS32: Introduction of floating point registers

This patch introduces floating point registers used for 32-bit
operations, and basic handling of FP values in operands. It is
partial work needed as a base for further work.
==========

to

==========
Subzero, MIPS32: Introduction of floating point registers

This patch introduces floating point registers used for 32-bit
operations, and basic handling of FP values in operands. It is
partial work needed as a base for further work.
==========

[obucinac, 2016-05-19 15:14:54]

obucinac@gmail.com changed reviewers:
+ sagar.thakur@imgtec.com, stichnot@chromium.org

[Jim Stichnoth, 2016-05-19 19:27:21]

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

[Jim Stichnoth, 2016-05-19 19:28:43]

I haven't reviewed this yet, but just wanted to ask that in the future, please
also include John and Karl as reviewers.  (I added them to this CL already, also
for future reference.)

[Jim Stichnoth, 2016-05-19 23:12:26]

I may have some more comments later on IceRegistersMIPS32.h, but I'll get the
rest out there now.

https://codereview.chromium.org/1993993004/diff/20001/src/IceInstMIPS32.def
File src/IceInstMIPS32.def (right):

https://codereview.chromium.org/1993993004/diff/20001/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))   \
This is just a suggestion:

With so many columns, I sometimes find it hard to figure out exactly which
column is which, within a given row.  I find it easier if I can construct some
kind of grouping through use of space characters.  For example:

  /* ... name, scratch,preserved,stackptr,frameptr, isInt,isI64Pair, ... */
  X(..., "f0", 1,0,0,0,0, 0,0,1, ...)

See for example IceInstX8632.def .

https://codereview.chromium.org/1993993004/diff/20001/src/IceInstMIPS32.def#n...
src/IceInstMIPS32.def:137: X(Reg_F23,      22,  "f23",   1, 0, 0, 0, 0, 0, 1, 0,
0, ALIASES1(Reg_F23))  \
Numbering got off here.  The 22 should be 23, and similar below.

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

https://codereview.chromium.org/1993993004/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:300: : InstMIPS32(Func, K, 2, Dest) {
The "2" is the max number of src operands.  I think it should be 1 here since
there is just a single addSource() call, also the emit routine asserts there is
just one src operand.

(This is in contrast to 2-operand instructions where one operand doubles as the
dest variable, like "opc a, b" with semantics "a = a opc b", in which case the
dest variable also needs to be added as a src operand.)

https://codereview.chromium.org/1993993004/diff/20001/src/IceRegistersMIPS32.h
File src/IceRegistersMIPS32.h (right):

https://codereview.chromium.org/1993993004/diff/20001/src/IceRegistersMIPS32....
src/IceRegistersMIPS32.h:65: // TODO(jvoung): Floating point and vector
registers...
Probably this TODO should be someone else's. :)

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

https://codereview.chromium.org/1993993004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:414: assert(Ty = IceType_f32);
Is this really necessary, given the UnimplementedError that comes next?

https://codereview.chromium.org/1993993004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1285: Reg = makeReg(Src0F->getType(),
RegMIPS32::Reg_V0);
Should this be Reg_F0 instead?

https://codereview.chromium.org/1993993004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1455: Reg = getPhysicalRegister(RegNum);
I'm very nervous about this use of getPhysicalRegister (as well as the place
below where this one was copied from).

getPhysicalRegister is supposed to be used just for "special" registers like
stack pointer, frame pointer, zero.  These are ones that aren't normally tracked
during liveness analysis and aren't assigned during register allocation.  I
suspect that using it here may cause liveness validation problems on larger
examples.

Can you just use makeReg(Ty, RegNum) regardless of RegNum.hasValue()?

https://codereview.chromium.org/1993993004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1467: (void)UpperBits;
Remove this

[obucinac, 2016-05-20 12:26:37]

John, this patch is not completed in any way, and I posted it mostly as the
reference for my colleagues, so they can see where I am going, since our work
overlaps at this point.

Right now, to make things faster and easier, I am considering using instruction
aliases that are supported in llvm. I need to check what aliases for loading
floats are supported in llvm version that is in our source tree. I work on some
of these, and I know for sure they exists, the question is do they exist in this
version. E.g., this is wip http://reviews.llvm.org/D14390, and will make things
much easier for us.

With instruction aliases we would emit single pseudo instruction, and pass the
hard work to llvm-mc (emit float as constant, register allocation, moving float
bit representation to gpr, moving from gpr to fpr), to expand it to a proper set
of instructions.

The point is, everything you see here regarding floats will probably change much
in the future.

[Jim Stichnoth, 2016-05-21 20:44:13]

On 2016/05/20 12:26:37, obucinac wrote:
> John, this patch is not completed in any way, and I posted it mostly as the
> reference for my colleagues, so they can see where I am going, since our work
> overlaps at this point.

(I assume you meant Jim, not John...)

This is fine, and my comments are mostly about structural issues that could
potentially cause problems in the future.

[Jim Stichnoth, 2016-05-21 21:02:25]

https://codereview.chromium.org/1993993004/diff/20001/src/IceRegistersMIPS32.h
File src/IceRegistersMIPS32.h (right):

https://codereview.chromium.org/1993993004/diff/20001/src/IceRegistersMIPS32....
src/IceRegistersMIPS32.h:69: assert(int(Reg_GPR_First) <= int(RegNum));
There is a subtle problem with the use of *_First and *_Last in these
calculations, here and below.

Specifically in the getEncoded*() functions, there is a built-in assumption that
our register enum values start with the register with the lowest encoded value,
and end with that with the highest encoded value.  This means that if we
change/reverse the order of register enum values, we have to know to make actual
code changes in the getEncoded*() functions.

Why would we bother changing the register enum order?  For ARM, John did just
that for float versus double FP registers.  All things equal, the register
allocator chooses the register with the lowest enum value.  Since float and
double registers alias each other, John choose to list float registers from low
to high encoding, and double register from high to low, to (hopefully) reduce
the incidence of conflicts.

Ultimately the x86 and ARM code was redone to use a table-based approach.  The
MIPS target was left alone, in part because only GPRs were represented to date. 
You don't need to do the same now, but I think a TODO should be added, to at
least consider that in the future.  May be best to defer that until some cross
tests are running.

[obucinac, 2016-05-27 11:13:19]

https://codereview.chromium.org/1993993004/diff/20001/src/IceInstMIPS32.def
File src/IceInstMIPS32.def (right):

https://codereview.chromium.org/1993993004/diff/20001/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))   \
On 2016/05/19 23:12:25, stichnot wrote:
> This is just a suggestion:
> 
> With so many columns, I sometimes find it hard to figure out exactly which
> column is which, within a given row.  I find it easier if I can construct some
> kind of grouping through use of space characters.  For example:
> 
>   /* ... name, scratch,preserved,stackptr,frameptr, isInt,isI64Pair, ... */
>   X(..., "f0", 1,0,0,0,0, 0,0,1, ...)
> 
> See for example IceInstX8632.def .

Acknowledged.

https://codereview.chromium.org/1993993004/diff/20001/src/IceInstMIPS32.def#n...
src/IceInstMIPS32.def:137: X(Reg_F23,      22,  "f23",   1, 0, 0, 0, 0, 0, 1, 0,
0, ALIASES1(Reg_F23))  \
On 2016/05/19 23:12:25, stichnot wrote:
> Numbering got off here.  The 22 should be 23, and similar below.

Done.

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

https://codereview.chromium.org/1993993004/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:300: : InstMIPS32(Func, K, 2, Dest) {
On 2016/05/19 23:12:25, stichnot wrote:
> The "2" is the max number of src operands.  I think it should be 1 here since
> there is just a single addSource() call, also the emit routine asserts there
is
> just one src operand.
> 
> (This is in contrast to 2-operand instructions where one operand doubles as
the
> dest variable, like "opc a, b" with semantics "a = a opc b", in which case the
> dest variable also needs to be added as a src operand.)

Done.

https://codereview.chromium.org/1993993004/diff/20001/src/IceRegistersMIPS32.h
File src/IceRegistersMIPS32.h (right):

https://codereview.chromium.org/1993993004/diff/20001/src/IceRegistersMIPS32....
src/IceRegistersMIPS32.h:65: // TODO(jvoung): Floating point and vector
registers...
On 2016/05/19 23:12:25, stichnot wrote:
> Probably this TODO should be someone else's. :)

Acknowledged.

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

https://codereview.chromium.org/1993993004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:414: assert(Ty = IceType_f32);
On 2016/05/19 23:12:25, stichnot wrote:
> Is this really necessary, given the UnimplementedError that comes next?

Done.

https://codereview.chromium.org/1993993004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1285: Reg = makeReg(Src0F->getType(),
RegMIPS32::Reg_V0);
On 2016/05/19 23:12:25, stichnot wrote:
> Should this be Reg_F0 instead?

We need to rework this to o32 convention. Leaving as is for now. o32 is our next
step.

https://codereview.chromium.org/1993993004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1455: Reg = getPhysicalRegister(RegNum);
On 2016/05/19 23:12:25, stichnot wrote:
> I'm very nervous about this use of getPhysicalRegister (as well as the place
> below where this one was copied from).
> 
> getPhysicalRegister is supposed to be used just for "special" registers like
> stack pointer, frame pointer, zero.  These are ones that aren't normally
tracked
> during liveness analysis and aren't assigned during register allocation.  I
> suspect that using it here may cause liveness validation problems on larger
> examples.
> 
> Can you just use makeReg(Ty, RegNum) regardless of RegNum.hasValue()?

We need to rework this to o32 convention. Leaving as is for now. o32 is our next
step.

https://codereview.chromium.org/1993993004/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:1467: (void)UpperBits;
On 2016/05/19 23:12:25, stichnot wrote:
> Remove this

Done.

[Jim Stichnoth, 2016-05-27 21:39:19]

lgtm

[Jim Stichnoth, 2016-05-27 21:40:35]

Description was changed from

==========
Subzero, MIPS32: Introduction of floating point registers

This patch introduces floating point registers used for 32-bit
operations, and basic handling of FP values in operands. It is
partial work needed as a base for further work.
==========

to

==========
Subzero, MIPS32: Introduction of floating point registers

This patch introduces floating point registers used for 32-bit
operations, and basic handling of FP values in operands. It is
partial work needed as a base for further work.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-05-27 21:40:36]

Committed patchset #6 (id:100001) manually as
2f593bbbfc5a20090faf90bdc665a2ccb447757f (presubmit successful).