Details
Diff Detail
Event Timeline
Thanks. There's quite a few comments below but most of it is about minor things. The main bit is the missing double-precision-on-fp64 case and the need to handle MIPS-I in some way (possibly just as an error).
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 3083 | There should be three possible TRUNC* instructions here since we have single-precision, double-precision-on-fp32, and double-precision-on-fp64. We also need either the expansion for the MIPS-I case or an error condition for MIPS-I. If you chose to make it an error, please add a test in test/MC/Mips/mips1/invalid-mips2.s (or invalid-mips2-wrong-error.s if the error message is wrong). | |
| lib/Target/Mips/MipsInstrFPU.td | ||
| 556–566 | This should be formatted consistently compared to the rest of the file. Unfortunately, clang-format doesn't support tablegen files so it has to be done manually. Could you name them PseudoTRUNC_W_S, and PseudoTRUNC_W_D32? We haven't been very consistent in naming pseudo instructions in the past but I've been trying to improve things for new pseudos. We also need a double-precision-on-fp64 variant with an FGR64Opnd instead of the AFGR64Opnd. Could you name it PseudoTRUNC_W_D? These need the HARDFLOAT adverb. The double precision ones also need FGR_32/FGR_64 adverbs for the AFGR64/FGR64 cases respectively. | |
| test/MC/Mips/mips32/valid.s | ||
| 203 | The encoding on the second one is missing. Since you're already adding trunc.* tests, could you also add the missing check for the two trunc.*'s above? These tests should be in all the test/MC/Mips/mips*/valid.s files but note that the expansion will be different for test/MC/Mips/mips1/valid.s. | |
| test/MC/Mips/mips64/valid.s | ||
| 284–287 | Since you're already adding trunc.* tests, could you also add the missing check for these four trunc.*'s? | |
Thanks. It will LGTM with a couple minor changes
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 3073 | Variables begin with a capital. Also, could you make it clear that is64Bit only refers to the FPU? It's possible to use 64-bit FPU's on an otherwise 32-bit CPU. | |
| 3091–3092 | I believe this nop should be unconditional. If I use gcc to assemble and disassemble: .set reorder trunc.w.s $f0, $f2, $2 foo: .set noreorder trunc.w.s $f0, $f2, $2 I get the same output for both expansions. | |
| 3096–3097 | Likewise | |
| 3102 | Like the other two, I don't think this requires isReorder(). I also don't think it's specific to trunc.w.d since I get a nop for both trunc.w.d and trunc.w.s when I use gcc to assemble and disassemble | |
| lib/Target/Mips/MipsInstrFPU.td | ||
| 556–566 |
This bit isn't formatted like the rest of the file yet. 'FGR32Opnd : $fd' is normally 'FGR32Opnd:$fd' and the arguments to MipsAsmPseudoInst<> normally align like a function. For example: def PseudoTRUNC_W_D32 : MipsAsmPseudoInst<(outs FGR32Opnd:$fd),
(ins AFGR64Opnd:$fs, GPR32Opnd:$rs),
"trunc.w.d\t$fd, $fs, $rs">,
FGR_32, HARDFLOAT;or def PseudoTRUNC_W_D32 :
MipsAsmPseudoInst<(outs FGR32Opnd:$fd), (ins AFGR64Opnd:$fs, GPR32Opnd:$rs),
"trunc.w.d\t$fd, $fs, $rs">,
FGR_32, HARDFLOAT; | |
| test/MC/Mips/mips1/valid.s | ||
| 139–140 ↗ | (On Diff #42296) | Alignment on the $4's |
| test/MC/Mips/mips32/valid.s | ||
| 203 |
Thanks | |
Thanks. It will LGTM with a couple minor changes
I've just noticed that this review doesn't have llvm-commits CC'd. I've added the list on this occasion but you'd normally be asked to create a new review with llvm-commits CC'd from the start. This is because adding llvm-commits later doesn't cause the whole patch to be sent to the list.
Variables begin with a capital.
Also, could you make it clear that is64Bit only refers to the FPU? It's possible to use 64-bit FPU's on an otherwise 32-bit CPU.