Handling of immediates bigger than 16 bits
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
There's quite a few comments here but most are about fairly small issues.
Resolves https://dmz-portal.mips.com/bugz/show_bug.cgi?id=2059
It appears that it also resolves:
https://dmz-portal.mips.com/bugz/show_bug.cgi?id=923
https://dmz-portal.mips.com/bugz/show_bug.cgi?id=689
As noted on D10537, the MIPS bugzilla isn't used by the LLVM community. Generally speaking, we shouldn't reference it.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
1650–1653 ↗ | (On Diff #27934) | I don't understand this check. All the above instructions have the form: insn reg, reg, imm Can you show me a case where it's false? |
1655 ↗ | (On Diff #27934) | Not quite. For ANDi, ORi, and XORi, the range will be 0 to 65535, but for ADDi, ADDiu, SLTi, SLTiu it will be -32768 to 32767. Also, use isInt<16>(ImmValue) and isUInt<16>(ImmValue) as appropriate. |
1656–1658 ↗ | (On Diff #27934) | Nit: Don't use redundant braces or else-after-return. |
2497 ↗ | (On Diff #27934) | What if ATReg is 0 because .set noat is in effect? |
2502 ↗ | (On Diff #27934) | The 'true' seems suspicious. Am I right in assuming support for the 64-bit equivalents will be in a later patch? |
2547–2551 ↗ | (On Diff #27934) | Nit: Avoid return-after-else like so: ... Instructions.push_back(tmpInst); return false } return true; |
test/MC/Mips/instalias-imm-expanding.s | ||
30 ↗ | (On Diff #27934) | Nit: Inconsistent indentation of the '# encoding'. By the way, I'm thinking about removing most of these '# encoding' checks. If we've tested it properly in test/MC/Mips/*/valid.s then we don't really need to do it again. That's something for later though. |
293–297 ↗ | (On Diff #27934) | Why? You don't check for it. |
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
1650–1653 ↗ | (On Diff #27934) | Check test/MC/Mips/xgot.s, line 36. |
2502 ↗ | (On Diff #27934) | Yes. |
LGTM with the remaining nits fixed.
I also noticed a few more on this read-through
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
1693–1700 ↗ | (On Diff #28954) | Nit: Indentation and else-after-return |
1696–1699 ↗ | (On Diff #28954) | The 'addiu $2, $2, %lo(_gp_disp)' ? Ok, that makes sense. |
1702–1704 ↗ | (On Diff #28954) | This has been marked done but the else-after-return is still there |
lib/Target/Mips/MipsInstrInfo.td | ||
1697–1698 ↗ | (On Diff #28954) | Indentation |