This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][Future] Add pld and pstd to future CPU
ClosedPublic

Authored by NeHuang on Jan 12 2020, 10:26 AM.

Details

Summary

Add the prefixed instructions pld and pstd to future CPU. These are load and store instructions that require new operand types that are 34 bits. This patch adds the two instructions as well as the operand types required.

Note that this patch also makes a minor change to tablegen to account for the fact that some instructions are going to require shifts greater than 31 bits for the new 34 bit instructions.

Diff Detail

Event Timeline

stefanp created this revision.Jan 12 2020, 10:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2020, 10:26 AM

Adding Florian, Daniel and Craig as reviewers since they were involved in the most recent change to this area of table gen.

jsji added a reviewer: Restricted Project.Jan 13 2020, 7:30 AM
jsji added a project: Restricted Project.

I'm not convinced you really need the FixedLenDecoderEmitter.cpp changes. PPCInstrFormats.td has bits<64> Inst in the I2 class, but PPCDisassembler.cpp has InsnType as being uint32_t . InsnType needs to be able to hold your biggest instruction for the generated code to work correctly. I believe that if you changed:

uint32_t Inst = ...;

in PPCDisassembler::getInstruction(), to:

uint64_t Inst = ...;

then you will no longer need to change FixedLenDecoderEmitter.cpp as you'll be able to use shifts of up to 63.

llvm/utils/TableGen/FixedLenDecoderEmitter.cpp
1115 ↗(On Diff #237550)

The OpInfo.begin()->Offset != 0 part doesn't look right to me at the moment. I think you're trying to guard against shift emitted by the code guarded by the else if (OpInfo.numFields() != 1 || EF.Offset != 0) on line 1133 (1120 in the old code) but here you only look at the first field whereas the original guard is checking all of them. Is there some sort order that guarantees that only the first field matters?

1120 ↗(On Diff #237550)

You can't really rely on sizeof(InsnType) being meaningful as InsnType is allowed to be an object. For example, that object might be using an APInt internally. You'll need to provide a template that can handle both integer and object InsnTypes.

NeHuang commandeered this revision.EditedJan 15 2020, 2:50 PM
NeHuang added a reviewer: stefanp.
NeHuang added a subscriber: NeHuang.

Commandeer the patch.

NeHuang updated this revision to Diff 238375.Jan 15 2020, 2:52 PM

Thanks @dsanders. Comment addressed.

amyk added a subscriber: amyk.Jan 16 2020, 4:04 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp
280

Do we need an assert here, as well?

llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
472

nit: spacing between OpNo + 1, and to make it consistent with printMemRegImm34 and the rest of the functions in this file.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp
168

s/immedaite/immediate.

llvm/test/MC/Disassembler/PowerPC/future-invalid.txt
1

Line too long?

NeHuang updated this revision to Diff 238912.Jan 17 2020, 3:38 PM
NeHuang marked an inline comment as done.

Addressed review comments above.

NeHuang marked 4 inline comments as done.Jan 17 2020, 3:38 PM
NeHuang added inline comments.
llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp
280

+1

nemanjai accepted this revision.Jan 27 2020, 8:53 AM

LGTM.

This revision is now accepted and ready to land.Jan 27 2020, 8:53 AM
This revision was automatically updated to reflect the committed changes.
NeHuang marked an inline comment as done.