This is an archive of the discontinued LLVM Phabricator instance.

[mips] Handling of immediates bigger than 16 bits
ClosedPublic

Authored by obucina on Jun 18 2015, 8:10 AM.

Details

Summary

Handling of immediates bigger than 16 bits

Diff Detail

Repository
rL LLVM

Event Timeline

obucina updated this revision to Diff 27934.Jun 18 2015, 8:10 AM
obucina retitled this revision from to [mips] Handling of immediates bigger than 16 bits.
obucina updated this object.
obucina edited the test plan for this revision. (Show Details)
obucina added reviewers: zoran.jovanovic, dsanders.
obucina added a subscriber: Unknown Object (MLST).
dsanders edited edge metadata.Jun 18 2015, 9:27 AM

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.

obucina marked 4 inline comments as done.Jul 2 2015, 8:44 AM
obucina added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1650–1653 ↗(On Diff #27934)

Check test/MC/Mips/xgot.s, line 36.

2502 ↗(On Diff #27934)

Yes.

obucina updated this revision to Diff 28950.Jul 2 2015, 9:02 AM
obucina updated this object.
obucina edited edge metadata.
obucina updated this revision to Diff 28954.Jul 2 2015, 9:08 AM
dsanders accepted this revision.Sep 7 2015, 5:31 AM
dsanders edited edge metadata.

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

This revision is now accepted and ready to land.Sep 7 2015, 5:31 AM
obucina updated this revision to Diff 34256.Sep 8 2015, 2:04 PM
obucina edited edge metadata.

New diff contains nit fixes and changes needed to apply and properly build this fix.

obucina updated this revision to Diff 34259.Sep 8 2015, 2:09 PM
obucina marked an inline comment as done.

New diff contains nit fixes and changes needed to apply and properly build this fix.

obucina marked 2 inline comments as done.Sep 11 2015, 12:08 PM
obucina marked 7 inline comments as done.Sep 16 2015, 6:10 AM

LGTM. Thanks

This revision was automatically updated to reflect the committed changes.