This is an archive of the discontinued LLVM Phabricator instance.

Differential D5567

Implement floating point compare for mips fast-isel
ClosedPublic

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

Details

Reviewers
dsanders
Commits
rG1f64ecab79df: Implement floating point compare for mips fast-isel
rL219530: Implement floating point compare for mips fast-isel
Summary

Expand SelectCmp to handle floating point compare

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
20

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

Done

20–27

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

Likewise below

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
rkotler mentioned this in D4137: Implement Mips fast isel for floating point compare..Dec 24 2014, 10:34 AM

Revision Contents

PathSize
lib/
Target/
Mips/
56 lines
test/
CodeGen/
Mips/
Fast-ISel/
182 lines

Diff 14300

lib/Target/Mips/MipsFastISel.cpp

Context not available.
RightReg = TempReg; RightReg = TempReg;
} }
unsigned ResultReg = createResultReg(&Mips::GPR32RegClass); unsigned ResultReg = createResultReg(&Mips::GPR32RegClass);
switch (CI->getPredicate()) { CmpInst::Predicate P = CI->getPredicate();
switch (P) {
default: default:
return false; return false;
case CmpInst::ICMP_EQ: { case CmpInst::ICMP_EQ: {
Context not available.
} }
case CmpInst::FCMP_OEQ: case CmpInst::FCMP_OEQ:
case CmpInst::FCMP_ONE: case CmpInst::FCMP_UNE:
case CmpInst::FCMP_OGT:
case CmpInst::FCMP_OLT: case CmpInst::FCMP_OLT:
case CmpInst::FCMP_OLE: case CmpInst::FCMP_OLE:
case CmpInst::FCMP_OGT:
case CmpInst::FCMP_OGE: {
bool IsFloat = Left->getType()->isFloatTy();
bool IsDouble = Left->getType()->isDoubleTy();
if (!IsFloat && !IsDouble)
return false;
unsigned Opc, CondMovOpc;
switch (P) {
case CmpInst::FCMP_OEQ:
Opc = IsFloat ? Mips::C_EQ_S : Mips::C_EQ_D32;
dsandersUnsubmitted
Not Done
ReplyInline Actions

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.

dsanders: C_EQ_D32 needs to be C_EQ_D64 for 64-bit FPU's and we don't guard against this. Similarly for…
dsandersUnsubmitted
Not Done
ReplyInline Actions

Done

dsanders: Done
CondMovOpc = Mips::MOVT_I;
break;
case CmpInst::FCMP_UNE:
Opc = IsFloat ? Mips::C_EQ_S : Mips::C_EQ_D32;
CondMovOpc = Mips::MOVF_I;
break;
case CmpInst::FCMP_OLT:
Opc = IsFloat ? Mips::C_OLT_S : Mips::C_OLT_D32;
CondMovOpc = Mips::MOVT_I;
break;
case CmpInst::FCMP_OLE:
Opc = IsFloat ? Mips::C_OLE_S : Mips::C_OLE_D32;
CondMovOpc = Mips::MOVT_I;
break;
case CmpInst::FCMP_OGT:
Opc = IsFloat ? Mips::C_OLE_S : Mips::C_OLE_D32;
CondMovOpc = Mips::MOVF_I;
break;
case CmpInst::FCMP_OGE:
Opc = IsFloat ? Mips::C_OLT_S : Mips::C_OLT_D32;
CondMovOpc = Mips::MOVF_I;
break;
dsandersUnsubmitted
Not Done
ReplyInline Actions

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.

dsanders: These two aren't correct when one of the operands is NaN. 'NaN ogt 0.0' is false because they…
dsandersUnsubmitted
Not Done
ReplyInline Actions

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: After chatting with Reed, I've taken a look at the SelectionDAG implementation and found that…
dsandersUnsubmitted
Not Done
ReplyInline Actions

Done

dsanders: Done
default:
break;
}
unsigned RegWithZero = createResultReg(&Mips::GPR32RegClass);
unsigned RegWithOne = createResultReg(&Mips::GPR32RegClass);
EmitInst(Mips::ADDiu, RegWithZero).addReg(Mips::ZERO).addImm(0);
EmitInst(Mips::ADDiu, RegWithOne).addReg(Mips::ZERO).addImm(1);
EmitInst(Opc).addReg(LeftReg).addReg(RightReg).addReg(
Mips::FCC0, RegState::ImplicitDefine);
MachineInstrBuilder MI = EmitInst(CondMovOpc, ResultReg)
.addReg(RegWithOne)
.addReg(Mips::FCC0)
.addReg(RegWithZero, RegState::Implicit);
MI->tieOperands(0, 3);
break;
}
case CmpInst::FCMP_ONE:
case CmpInst::FCMP_UEQ: case CmpInst::FCMP_UEQ:
case CmpInst::FCMP_UNE:
case CmpInst::FCMP_UGT: case CmpInst::FCMP_UGT:
case CmpInst::FCMP_ULT: case CmpInst::FCMP_ULT:
case CmpInst::FCMP_UGE: case CmpInst::FCMP_UGE:
Context not available.

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

  • This file was added.
; RUN: llc -march=mipsel -relocation-model=pic -O0 -mips-fast-isel -fast-isel-abort -mcpu=mips32r2 \
; RUN: < %s | FileCheck %s
; RUN: llc -march=mipsel -relocation-model=pic -O0 -mips-fast-isel -fast-isel-abort -mcpu=mips32 \
; RUN: < %s | FileCheck %s
@f1 = common global float 0.000000e+00, align 4
@f2 = common global float 0.000000e+00, align 4
@b1 = common global i32 0, align 4
@d1 = common global double 0.000000e+00, align 8
@d2 = common global double 0.000000e+00, align 8
; Function Attrs: nounwind
define void @feq1() {
entry:
%0 = load float* @f1, align 4
%1 = load float* @f2, align 4
%cmp = fcmp oeq float %0, %1
; CHECK-LABEL: feq1:
; CHECK: c.eq.s $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movt ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
dsandersUnsubmitted
Not Done
ReplyInline Actions

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

Likewise for the tests below.

dsanders: We ought to check that the registers are in the right order. Likewise for the tests below.
dsandersUnsubmitted
Not Done
ReplyInline Actions

Done

dsanders: Done
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}
; Function Attrs: nounwind
dsandersUnsubmitted
Not Done
ReplyInline Actions

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

Likewise below

dsanders: Formatting nit: Could you line up the start of each instruction? Likewise below
define void @fne1() {
entry:
%0 = load float* @f1, align 4
%1 = load float* @f2, align 4
%cmp = fcmp une float %0, %1
; CHECK-LABEL: fne1:
; CHECK: c.eq.s $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movf ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}
; Function Attrs: nounwind
define void @flt1() {
entry:
%0 = load float* @f1, align 4
%1 = load float* @f2, align 4
%cmp = fcmp olt float %0, %1
; CHECK-LABEL: flt1:
; CHECK: c.olt.s $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movt ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}
; Function Attrs: nounwind
define void @fgt1() {
entry:
%0 = load float* @f1, align 4
%1 = load float* @f2, align 4
%cmp = fcmp ogt float %0, %1
; CHECK-LABEL: fgt1:
; CHECK: c.ole.s $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movf ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}
; Function Attrs: nounwind
define void @fle1() {
entry:
%0 = load float* @f1, align 4
%1 = load float* @f2, align 4
%cmp = fcmp ole float %0, %1
; CHECK-LABEL: fle1:
; CHECK: c.ole.s $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movt ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}
; Function Attrs: nounwind
define void @fge1() {
entry:
%0 = load float* @f1, align 4
%1 = load float* @f2, align 4
%cmp = fcmp oge float %0, %1
; CHECK-LABEL: fge1:
; CHECK: c.olt.s $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movf ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}
; Function Attrs: nounwind
define void @deq1() {
entry:
%0 = load double* @d1, align 8
%1 = load double* @d2, align 8
%cmp = fcmp oeq double %0, %1
; CHECK-LABEL: deq1:
; CHECK: c.eq.d $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movt ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}
; Function Attrs: nounwind
define void @dne1() {
entry:
%0 = load double* @d1, align 8
%1 = load double* @d2, align 8
%cmp = fcmp une double %0, %1
; CHECK-LABEL: dne1:
; CHECK: c.eq.d $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movf ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}
; Function Attrs: nounwind
define void @dlt1() {
entry:
%0 = load double* @d1, align 8
%1 = load double* @d2, align 8
%cmp = fcmp olt double %0, %1
; CHECK-LABEL: dlt1:
; CHECK: c.olt.d $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movt ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}
; Function Attrs: nounwind
define void @dgt1() {
entry:
%0 = load double* @d1, align 8
%1 = load double* @d2, align 8
%cmp = fcmp ogt double %0, %1
; CHECK-LABEL: dgt1:
; CHECK: c.ole.d $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movf ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}
; Function Attrs: nounwind
define void @dle1() {
entry:
%0 = load double* @d1, align 8
%1 = load double* @d2, align 8
%cmp = fcmp ole double %0, %1
; CHECK-LABEL: dle1:
; CHECK: c.ole.d $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movt ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}
; Function Attrs: nounwind
define void @dge1() {
entry:
%0 = load double* @d1, align 8
%1 = load double* @d2, align 8
%cmp = fcmp oge double %0, %1
; CHECK-LABEL: dge1:
; CHECK: c.olt.d $f{{[0-9]+}}, $f{{[0-9]+}}
; CHECK: movf ${{[0-9]+}}, ${{[0-9]+}}, $fcc0
%conv = zext i1 %cmp to i32
store i32 %conv, i32* @b1, align 4
ret void
}