This is an archive of the discontinued LLVM Phabricator instance.

[mips] Range check uimm16 and fix several bugs this revealed.
ClosedPublic

Authored by dsanders on Dec 11 2015, 3:14 AM.

Details

Summary

The bugs were:

  • teq and similar take 4-bit unsigned immediates on microMIPS.
  • teqi and similar have side-effects like teq do.
  • shll_s.w and shra_r.w take 5-bit unsigned immediates.
  • The various DSP ext* instructions take a 5-bit immediate.
  • repl.qh takes an 8-bit unsigned immediate.
  • repl.ph takes a 10-bit unsigned immediate.
  • rddsp/wrdsp take a 10-bit unsigned immediate.
  • teqi and similar take signed 16-bit immediates (10-bit for microMIPS).
  • Out-of-range immediate macros for or/xor take a simm32/simm64 depending on architecture. I'll fix the simm64 case properly when I reach simm32.

lui is a bit more lenient than GAS and accepts signed immediates in addition
to unsigned. This is because MipsMCExpr can produce signed values when
constant folding and it currently lacks a way of knowing it should fold to
an unsigned value.

Diff Detail

Event Timeline

dsanders updated this revision to Diff 42513.Dec 11 2015, 3:14 AM
dsanders retitled this revision from to [mips] Range check uimm16 and fix several bugs this revealed..
dsanders updated this object.
dsanders added a reviewer: vkalintiris.
dsanders added a subscriber: llvm-commits.
vkalintiris edited edge metadata.Dec 20 2015, 4:37 PM

Almost everything looks good to me. However, I can not get the GNU assembler to accept the new lui tests that you changed. Should I use any specific option that I'm not aware of, or perhaps a more recent GCC version?

Almost everything looks good to me. However, I can not get the GNU assembler to accept the new lui tests that you changed. Should I use any specific option that I'm not aware of, or perhaps a more recent GCC version?

Well spotted, I hadn't noticed that. We should match the range that GAS accepts but I've also queried whether GAS is doing the right thing since simm16 is closer to what the instruction does (the result is sign extended from 32-bit to register width). At the moment, I think it should accept both simm16 and uimm16 (like addiu) and coerce it to simm16.

I'll correct this patch shortly.

dsanders updated this revision to Diff 45537.Jan 21 2016, 8:28 AM
dsanders updated this object.
dsanders edited edge metadata.

Fix issue with emitting signed values for lui when GAS expects unsigned.

IAS is a bit more lenient and accepts either so that immediates produced by
evaluating MipsMCExpr's involving constant symbols are accepted.
vkalintiris accepted this revision.Feb 1 2016, 3:34 AM
vkalintiris edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Feb 1 2016, 3:34 AM
dsanders closed this revision.Feb 1 2016, 7:17 AM