This is an archive of the discontinued LLVM Phabricator instance.

[mips] Handle 64 bit immediate in and/or/xor pseudo instructions on mips64
ClosedPublic

Authored by arichardson on Feb 23 2017, 6:09 AM.

Details

Summary

Previously LLVM was assuming 32-bit signed immediates which results in and with a bitmask that has bit 31 set to incorrectly include bits 63-32 in the result.
After applying this patch I can now compile all of the FreeBSD mips assembly code with clang.

This issue also affects the nor, slt and sltu macros and I will fix those in a separate review.

Diff Detail

Repository
rL LLVM

Event Timeline

arichardson created this revision.Feb 23 2017, 6:09 AM

rebased on master

sdardis edited edge metadata.Feb 23 2017, 11:54 AM

Hi,

Thanks for taking a look at this, I was about to start on it today, but your patch arrived first.

I am not sure what the correct approach is but it seems I need to add the GPR_32 constraint because otherwise the 32 bit aliases will be used for 64 bit code generation.

Yes, this approach is correct. The asm parser considers the predicates of the instruction definition to chose which entry in the matcher table should be considered.

This issue also affects the nor, slt and sltu macros but I don't know how to change the tablegen to make these work.

I've inlined a comment on how to get slt and sltu macros working. They're different from the rest of the instructions you're changing due to legalization issues.

To handle NorImm, change the def of NORImm into a class NORIMM_DESC_BASE which takes two parameters, RegisterOperand and DAGOperand which are then used instead of GPR32Opnd and simm32_relaxed. You can then introduce a def of NORImm which takes GPR32Opnd and simm32_relaxed, and a def NORImm64 which takes GPR64Opnd and imm64. These two instructions will need GPR_32 and GPR_64 predicates attached to them otherwise the result of parsing 'nor $4, $5, -1' will give results based on the order of the matcher table.

For the TableGen side of things, you can look at the class LoadImmediate32 and how it is used with def LoadImm32 just below it for a reference. You'll also need to look at where NORImm get handled in the cpp side and provide similar support for NORImm64.

Thanks again,
Simon

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3830–3847 ↗(On Diff #89516)

For the 64 bit case, you need to pick the corresponding 64 bit instruction definition. Picking the 32 bit version introduces a very subtle bug in that the instruction emitted will be (harmless) malformed.

lib/Target/Mips/MipsInstrInfo.td
2324–2327 ↗(On Diff #89516)

Add another parameter here which defaults to RO, use it with $rs.

Then you can modify just the macro definitions for slt and sltu to produce a result as a GPR32Opnd.

test/MC/Mips/mips64-instalias-imm-expanding.s
381–383 ↗(On Diff #89516)

This should not be sign extended.

469 ↗(On Diff #89516)

Change that XXXAR to FIXME, so I can find it later.

We currently don't do many possible optimizations regarding constant synthesis in the assembler. I'd like to improve that but other things have higher priority.

arichardson edited the summary of this revision. (Show Details)

fixed issues with and/or/xor. Will add a separate review for nor/slt/sltu later today.

sdardis accepted this revision.Feb 24 2017, 5:27 AM

LGTM to with inline comment addressed.

lib/Target/Mips/Mips64InstrInfo.td
710–715 ↗(On Diff #89631)

These need to be guarded by GPR_64.

This revision is now accepted and ready to land.Feb 24 2017, 5:27 AM
arichardson marked 2 inline comments as done.

Fixed inline issues.

I don't have commit access, could you submit for me?

This revision was automatically updated to reflect the committed changes.