This is an archive of the discontinued LLVM Phabricator instance.

[mips] [IAS] Fix expansion of negative 32-bit immediates for LI/DLI.
ClosedPublic

Authored by tomatabacu on Mar 27 2015, 6:47 AM.

Details

Summary

To maintain compatibility with GAS, we need to stop treating negative 32-bit immediates as 64-bit values when expanding LI/DLI.
This currently happens because of sign extension.

To do this we need to choose the 32-bit value expansion for values which use their upper 33 bits only for sign extension (i.e. no 0's, only 1's).

Diff Detail

Event Timeline

tomatabacu updated this revision to Diff 22792.Mar 27 2015, 6:47 AM
tomatabacu retitled this revision from to [mips] [IAS] Fix negation of 32-bit immediates..
tomatabacu updated this object.
tomatabacu edited the test plan for this revision. (Show Details)
tomatabacu added a reviewer: dsanders.
tomatabacu added a subscriber: Unknown Object (MLST).
dsanders edited edge metadata.Apr 7 2015, 7:10 AM

I think we're attacking this in the wrong place at the moment. It's difficult to tell without going deep into the guts of the matcher but it's important to the operation of the parser that we don't specialize our operands too early since there's no facility for backtracking.

This was a particular problem for floating point registers a while back. The parser would identify register names like '$f0' as being an F0_64 and having committed to that interpretation would then fail to match FGR32 versions of the instructions. In our current implementation, our floating point operand for $f0 is all three of F0, D0, and F0_64 (it's stored as a register index that permits FGR32, FGR64, and AFGR64 contexts) until the operand is rendered into an instruction which specializes it to a single register. The same needs to be true of integers, we need to have a generic 64-bit integer which is specialized into other integer types when it's rendered into an instruction.

If the instruction were expanded by the tablegen-erated matcher, we could attack this in tablegen using an AsmOperandClass. The PredicateMethod would determine whether the value was a valid unsigned/signed 32-bit integer (which is the case when it fits in a _33_-bit signed integer). Then if the instruction matched, the RenderMethod would adjust the value so that the excess bits are a sign-extension of the 32-bit value. So for example, 0x0000000080000000 would return true for the PredicateMethod and be converted to 0xffffffff80000000 by the RenderMethod.

I'd therefore suggest that expandLoadImm() have some way of knowing whether the context for the integer is 32-bit or 64-bit. When called in a 32-bit context, it would convert the value to a signed 32-bit integer before emitting the code to produce it.

tomatabacu updated this revision to Diff 23838.Apr 16 2015, 3:52 AM
tomatabacu retitled this revision from [mips] [IAS] Fix negation of 32-bit immediates. to [mips] [IAS] Fix expansion of negative 32-bit immediates for LI/DLI..
tomatabacu updated this object.
tomatabacu edited edge metadata.

Took a new approach to solving the problem.
Updated title and summary to reflect code changes.
Updated tests.

Thanks for reworking this. A couple comments:

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1744–1746

This condition is a complicated way of saying 'does it fit in a signed/unsigned 32-bit integer?' which can be more simply written as 'isInt<32>(ImmValue) || isUInt<32>(ImmValue)'. A few of the other paths do the same kind of thing, for example the first if-statement could be written 'isUInt<16>(ImmValue).

Also: it isn't correct for the DLI case when ImmValue is 0x80000000 because 'lui' produces a sign-extended (32->64-bit) result giving 0xffffffff80000000 in the register. It's fine for 32-bit though.
You need the condition to be isInt<32>(ImmValue) for the DLI case and 'isInt<32>(ImmValue) || isUInt<32>(ImmValue)' for the LI case.

1765

I know this isn't from your patch but I just noticed it. Shouldn't these be '16-bits'?

1791

Same here

Replied to inline comments.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1765

Yes. Fixed in r235891.

1791

Yes. Fixed in r235891.

tomatabacu updated this revision to Diff 24619.Apr 29 2015, 5:43 AM

Switched to using the templates from MathExtras.h.
Switched to using ORi instead of LUi for DLI, to avoid sign-extension.
Rebased on top of D8974, D9289 and D9290 to make things easier to do.

Will switch the other paths to use the templates from MathExtras.h in a follow-up patch.

tomatabacu updated this revision to Diff 25146.May 7 2015, 3:06 AM

Rebased on top of new D9290.

dsanders accepted this revision.May 8 2015, 4:01 AM
dsanders edited edge metadata.

With a couple corrections it will LGTM

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1752

This isn't quite right. You need to check for `!isInt<32>(ImmValue)`. That way it will use lui+ori except when it really needs to use ori+dslli+ori such as for 0x80000000.

You'll also need a test to cover this once the check is corrected.

test/MC/Mips/mips64-expansions.s
2–3

This was correct before. 0x10002 should use:
lui ..., 0x1
ori ..., ..., 0x2

2–3

This was correct before. 0x10000 should use
lui ..., 0x1

This revision is now accepted and ready to land.May 8 2015, 4:01 AM
tomatabacu updated this revision to Diff 25769.May 14 2015, 6:38 AM
tomatabacu edited edge metadata.

Made the requested changes.

tomatabacu closed this revision.May 15 2015, 2:45 AM
This revision was automatically updated to reflect the committed changes.