Page MenuHomePhabricator

Implement floating point compare for mips fast-isel
ClosedPublic

Authored by rkotler on Oct 1 2014, 5:14 PM.

Diff Detail

Event Timeline

rkotler updated this revision to Diff 14300.Oct 1 2014, 5:14 PM
rkotler retitled this revision from to Implement floating point compare for mips fast-isel.
rkotler updated this object.
rkotler edited the test plan for this revision. (Show Details)
rkotler added a reviewer: dsanders.
rkotler added subscribers: mcrosier, rfuhler, Unknown Object (MLST).
mcrosier removed a subscriber: mcrosier.Oct 2 2014, 7:34 AM
dsanders requested changes to this revision.Oct 3 2014, 3:33 AM
dsanders edited edge metadata.

I'm holding back on the conditional LGTM for now since I would like to see how you solve the 64-bit FPU cases both here and in D5562 first.

lib/Target/Mips/MipsFastISel.cpp
625

C_EQ_D32 needs to be C_EQ_D64 for 64-bit FPU's and we don't guard against this.

Similarly for the other compares.

640–647

These two aren't correct when one of the operands is NaN. 'NaN ogt 0.0' is false because they are unordered, but '!(NaN ole 0.0)' is true. Swap the LHS/RHS instead.

test/CodeGen/Mips/Fast-ISel/fpcmpa.ll
21

We ought to check that the registers are in the right order.

Likewise for the tests below.

This revision now requires changes to proceed.Oct 3 2014, 3:33 AM
rkotler updated this revision to Diff 14539.Oct 7 2014, 5:45 PM
rkotler edited edge metadata.

Fix patch per pre-commit recommendations.

dsanders added inline comments.Oct 8 2014, 2:26 AM
lib/Target/Mips/MipsFastISel.cpp
640–647

After chatting with Reed, I've taken a look at the SelectionDAG implementation and found that it is using c.ule.[ds] and c.ule.[ds] for FCMP_OGT and FCMP_OGE respectively and then inverting the boolean result before the value is consumed. This is valid too and we should probably go with that for the sake of consistency.

dsanders accepted this revision.Oct 8 2014, 6:45 AM
dsanders edited edge metadata.

LGTM with one new formatting nit.

lib/Target/Mips/MipsFastISel.cpp
625

Done

640–647

Done

test/CodeGen/Mips/Fast-ISel/fpcmpa.ll
20–27

Formatting nit: Could you line up the start of each instruction?

Likewise below

21

Done

This revision is now accepted and ready to land.Oct 8 2014, 6:45 AM
rkotler updated this revision to Diff 14747.Oct 10 2014, 1:21 PM
rkotler edited edge metadata.

Make instructions line up properly in the test case.

rkotler closed this revision.Oct 10 2014, 1:56 PM