Subzero, MIPS32: Floating point comparison

Subzero, MIPS32: Floating point comparison

This patch implements lowerFcmp, for lowering floating point comparison.

R=stichnot@chromium.org

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

[obucinac, 2016-06-07 18:57:13]

Description was changed from

==========
Subzero, MIPS32: Floating point comparison

This patch implements lowerFcmp, for lowering floating point comparison.

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

to

==========
Subzero, MIPS32: Floating point comparison

This patch implements lowerFcmp, for lowering floating point comparison.

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

[obucinac, 2016-06-07 18:57:13]

obucinac@gmail.com changed reviewers:
+ mohit.bhakkad@imgtec.com, sagar.thakur@imgtec.com

[obucinac, 2016-06-07 18:57:47]

On 2016/06/07 18:57:13, obucinac wrote:
> mailto:obucinac@gmail.com changed reviewers:
> + mailto:mohit.bhakkad@imgtec.com, mailto:sagar.thakur@imgtec.com

WIP, waiting for prerequisites (CC and FP register reservation).

[obucinac, 2016-09-08 09:34:51]

Description was changed from

==========
Subzero, MIPS32: Floating point comparison

This patch implements lowerFcmp, for lowering floating point comparison.

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

to

==========
Subzero, MIPS32: Floating point comparison

This patch implements lowerFcmp, for lowering floating point comparison.
==========

[obucinac, 2016-09-08 09:34:51]

obucinac@gmail.com changed reviewers:
+ eholk@chromium.org, jaydeep.patil@imgtec.com, jpp@chromium.org,
kschimpf@google.com, stichnot@chromium.org
- mohit.bhakkad@imgtec.com

[obucinac, 2016-09-08 09:35:11]

On 2016/06/07 18:57:47, obucinac wrote:
> On 2016/06/07 18:57:13, obucinac wrote:
> > mailto:obucinac@gmail.com changed reviewers:
> > + mailto:mohit.bhakkad@imgtec.com, mailto:sagar.thakur@imgtec.com
> 
> WIP, waiting for prerequisites (CC and FP register reservation).

This patch is ready for review now.

[Jim Stichnoth, 2016-09-12 14:14:41]

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

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:87: Ostream &Str = Func->getContext()->getStrEmit();
Add the usual pattern at the beginning of emit() and dump().

  if (!BuildDefs.dump())
    return;

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:89: Str << "$fcc" << (uint16_t)FCC;
static_cast<>

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:102: OperandMIPS32FCC(Cfg *Func, Type Ty,
OperandMIPS32FCC::FCC FCC)
Optional:
s/Func//
Then you can remove the void-cast of Func below.

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:103: : OperandMIPS32(kFCC, Ty), FCC(FCC) {
I don't think the Type makes much sense here, especially since (assuming I
didn't miss something) the Type argument is always IceType_i32.

I would consider removing the Type argument, and maybe pass IceType_void to the
parent ctor.

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:107: OperandMIPS32FCC::FCC FCC;
const

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:859: template <InstMIPS32::InstKindMIPS32 K>
You may want to consider defining a single InstMIPS32FPCmp class, where the
condition code is stored as a class member, rather than templatizing it with the
condition code.  The latter may increase the code size of the class by 14x
(unless the compiler that builds Subzero does something clever, which I kind of
doubt).

The drawback of the parameterized approach is that you probably need to add the
x-macro machinery to IceInstMIPS32.def which would provide the mapping between
enum values, textual opcode strings, and operand types.

(This is just a suggestion, final decision is yours.)

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

https://codereview.chromium.org/2047043002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2055: 
delete this blank line?

https://codereview.chromium.org/2047043002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2072: Operand *FCC0 =
OperandMIPS32FCC::create(getContext().getNode()->getCfg(),
You can just use TargetLowering::getFunc() here.

https://codereview.chromium.org/2047043002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2087: case InstFcmp::Oeq: {
Does the MIPS instruction set really provide a 1:1 mapping for the 14
ordered/unordered LLVM fcmp conditions?  If so, that is amazing.  See for
example the complex lowering that ARM and x86 have to do for the NaN situations.

In any case, I'll have full confidence once the test_fcmp cross test is running.

[obucinac, 2016-09-12 19:16:16]

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

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:87: Ostream &Str = Func->getContext()->getStrEmit();
On 2016/09/12 14:14:40, stichnot wrote:
> Add the usual pattern at the beginning of emit() and dump().
> 
>   if (!BuildDefs.dump())
>     return;

Done.

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:89: Str << "$fcc" << (uint16_t)FCC;
On 2016/09/12 14:14:40, stichnot wrote:
> static_cast<>

Done.

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:102: OperandMIPS32FCC(Cfg *Func, Type Ty,
OperandMIPS32FCC::FCC FCC)
On 2016/09/12 14:14:40, stichnot wrote:
> Optional:
> s/Func//
> Then you can remove the void-cast of Func below.

Done.

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:103: : OperandMIPS32(kFCC, Ty), FCC(FCC) {
On 2016/09/12 14:14:41, stichnot wrote:
> I don't think the Type makes much sense here, especially since (assuming I
> didn't miss something) the Type argument is always IceType_i32.
> 
> I would consider removing the Type argument, and maybe pass IceType_void to
the
> parent ctor.

Done.

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:107: OperandMIPS32FCC::FCC FCC;
On 2016/09/12 14:14:40, stichnot wrote:
> const

Done.

https://codereview.chromium.org/2047043002/diff/20001/src/IceInstMIPS32.h#new...
src/IceInstMIPS32.h:859: template <InstMIPS32::InstKindMIPS32 K>
On 2016/09/12 14:14:40, stichnot wrote:
> You may want to consider defining a single InstMIPS32FPCmp class, where the
> condition code is stored as a class member, rather than templatizing it with
the
> condition code.  The latter may increase the code size of the class by 14x
> (unless the compiler that builds Subzero does something clever, which I kind
of
> doubt).
> 
> The drawback of the parameterized approach is that you probably need to add
the
> x-macro machinery to IceInstMIPS32.def which would provide the mapping between
> enum values, textual opcode strings, and operand types.
> 
> (This is just a suggestion, final decision is yours.)

I would like to leave it as it is, for the consistency with other instruction
implementations.

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

https://codereview.chromium.org/2047043002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2055: 
On 2016/09/12 14:14:41, stichnot wrote:
> delete this blank line?

Done.

https://codereview.chromium.org/2047043002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2072: Operand *FCC0 =
OperandMIPS32FCC::create(getContext().getNode()->getCfg(),
On 2016/09/12 14:14:41, stichnot wrote:
> You can just use TargetLowering::getFunc() here.

Done.

https://codereview.chromium.org/2047043002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2087: case InstFcmp::Oeq: {
On 2016/09/12 14:14:41, stichnot wrote:
> Does the MIPS instruction set really provide a 1:1 mapping for the 14
> ordered/unordered LLVM fcmp conditions?  If so, that is amazing.  See for
> example the complex lowering that ARM and x86 have to do for the NaN
situations.
> 
> In any case, I'll have full confidence once the test_fcmp cross test is
running.

This is related link:

https://imagination-technologies-cloudfront-assets.s3.amazonaws.com/documenta...

Page number 108.

MIPS instruction C.cond.fmt have in total 48 variants, 16 variants per type,
where we are using only some variants for 32bit and 64bit floats. If I
understand instruction reference correctly, I believe that there is a 1:1 match.
Some of them are achieved by negating comparison result, which is basically done
by movt and movf conditional moves.

To make this change, I compiled test file with llc for mips32, disassembled it
and followed the output.

I will try to merge this patch with crosstest patch, to see results, and report
them back here.

[obucinac, 2016-09-12 20:13:36]

In short, running test_fcmp crosstest gives this result

...
TotalTests=123904 Passes=99264 Failures=24640

I merged FP comparison patch, Crosstest patch, and still unfinished, but
working, patch for lowerSelect (I will upload it shortly).

With all this, and disabled tests for vector, I was able to run test_fcmp
crosstest, and got these results.

"cat output.txt | cut -c1-7 | sort | uniq" shows that all comparisons fail for
some parameter values.

A huge number of passed tests, and all comparisons failing for some values may
indicate that comparison instructions is not the only place where we should look
for problems.

[Jim Stichnoth, 2016-09-13 12:45:58]

lgtm

[Jim Stichnoth, 2016-09-13 12:46:32]

Description was changed from

==========
Subzero, MIPS32: Floating point comparison

This patch implements lowerFcmp, for lowering floating point comparison.
==========

to

==========
Subzero, MIPS32: Floating point comparison

This patch implements lowerFcmp, for lowering floating point comparison.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-09-13 12:46:33]

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