Page MenuHomePhabricator

[mips] Addition of the immediate cases for the instructions [d]div, [d]divu
ClosedPublic

Authored by sdardis on Feb 4 2016, 8:25 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

obucina updated this revision to Diff 46924.Feb 4 2016, 8:25 AM
obucina retitled this revision from to [mips] Addition of the immediate cases for the instructions [d]div, [d]divu.
obucina updated this object.
obucina added reviewers: dsanders, zoran.jovanovic.
obucina added a subscriber: llvm-commits.
dsanders edited edge metadata.Feb 12 2016, 9:31 AM

It looks like this patch also contains D16888. Could you reduce it to just the changes for this patch?

obucina updated this revision to Diff 49173.Feb 26 2016, 4:46 AM
obucina edited edge metadata.

Patch reduced to contain only changes for adding immediate operand for DIV macro.

dsanders accepted this revision.Mar 11 2016, 6:09 AM
dsanders edited edge metadata.

LGTM with a couple small changes

lib/Target/Mips/MipsInstrInfo.td
2066 ↗(On Diff #49173)

I believe this simm16 should be simm32. It currently works because I haven't added range checking to simm16 yet.

Similarly for the other 32-bit divides below.

2093 ↗(On Diff #49173)

I believe this simm16 should be simm64. It currently works because I haven't added range checking to simm16 yet.

Similarly for the other 64-bit divides below.

test/MC/Mips/macro-ddiv.s
83–87 ↗(On Diff #49173)

Could you add a 64-bit constant too?

185 ↗(On Diff #49173)

From this point onwards, the tests look like duplicates of the first half but checking the trap case. In a separate patch, could you fold these together so each one looks something like:

ddiv $4, 0
# CHECK-NOTRAP: break 7                   # encoding: [0x00,0x07,0x00,0x0d]
# CHECK-TRAP: teq $zero, $zero, 7         # encoding: [0x00,0x00,0x01,0xf4]

Likewise for the other division macro test files in this review.

This revision is now accepted and ready to land.Mar 11 2016, 6:09 AM
obucina updated this revision to Diff 50734.Mar 15 2016, 8:14 AM
obucina edited edge metadata.

Adding 64bit constants to test cases.
Macro definitions updated to simm32, imm64
Tests reorganized.

obucina marked 3 inline comments as done.Mar 15 2016, 8:19 AM
obucina added inline comments.Mar 15 2016, 8:57 AM
lib/Target/Mips/MipsInstrInfo.td
2093 ↗(On Diff #50734)

In llvm, simm64 is not defined. Only imm64 is allowed.

test/MC/Mips/macro-ddiv.s
121–125 ↗(On Diff #50734)

In gcc, for constants larger than 32 bits, we get the error message:
"Error: number (0x0000000fffffffff) larger than 32 bits".

Do we need to get the same error in llvm for 64bit constants?

dsanders added inline comments.Mar 15 2016, 9:27 AM
lib/Target/Mips/MipsInstrInfo.td
2093 ↗(On Diff #50734)

The majority of these operands are target defined. For example, simm32 is:

def simm32      : Operand<i32>;
test/MC/Mips/macro-ddiv.s
121–125 ↗(On Diff #50734)

I don't get this error:

$ cat t.s
ddiv $2, $3, 0xffffffff
$ mips-mti-linux-gnu-gcc -c -o t.o t.s -mips64r2
obucina added inline comments.Mar 23 2016, 8:34 AM
test/MC/Mips/macro-ddiv.s
121–125 ↗(On Diff #50734)

You have eight Fs, we have nine. Try with nine.

dsanders added inline comments.Mar 23 2016, 9:29 AM
test/MC/Mips/macro-ddiv.s
121–125 ↗(On Diff #50734)

These all work for me:

ddiv $2, $3, 0xf
ddiv $2, $3, 0xff
ddiv $2, $3, 0xfff
ddiv $2, $3, 0xffff
ddiv $2, $3, 0xfffff
ddiv $2, $3, 0xffffff
ddiv $2, $3, 0xfffffff
ddiv $2, $3, 0xffffffff
ddiv $2, $3, 0xfffffffff
ddiv $2, $3, 0xffffffffff
ddiv $2, $3, 0xfffffffffff
obucina updated this revision to Diff 52044.Mar 30 2016, 6:19 AM

Adding 64bit constants to ddiv[u] test cases.

obucina marked 7 inline comments as done.Mar 30 2016, 6:20 AM
obucina added inline comments.
test/MC/Mips/macro-ddiv.s
185–189 ↗(On Diff #52044)

With MTI toolchain everything works fine.

sdardis commandeered this revision.Jan 30 2017, 9:07 AM
sdardis added a reviewer: obucina.

Taking over this patch.

This revision was automatically updated to reflect the committed changes.
sdardis marked an inline comment as done.