Page MenuHomePhabricator

[PowerPC][Future] Add prefixed instruction paddi to future CPU
ClosedPublic

Authored by NeHuang on Jan 12 2020, 7:55 AM.

Details

Summary

Future CPU will include support for prefixed instructions. These prefixed instructions are formed by a 4 byte prefix immediately followed by a 4 byte instruction effectively making an 8 byte instruction. The new instruction paddi is a prefixed form of addi.

This patch adds paddi and all of the support required for that instruction. The majority of the patch deals with supporting the new prefixed instructions. The addition of paddi is mainly to allow for testing.

Diff Detail

Event Timeline

stefanp created this revision.Jan 12 2020, 7:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2020, 7:55 AM
nemanjai added a reviewer: Restricted Project.Jan 12 2020, 9:24 AM

Missing context on this review. Please produce the diff with -U99999 or arc to ensure full context is available.

nemanjai added inline comments.Jan 12 2020, 9:53 AM
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
72

The definition above hard codes the prefix. I don't see a reason for this one to be any different.

stefanp updated this revision to Diff 237547.Jan 12 2020, 10:01 AM

Added full context.

jsji added a project: Restricted Project.Jan 13 2020, 7:31 AM
stefanp updated this revision to Diff 237763.EditedJan 13 2020, 1:09 PM

Addressed review comment by hard coding the prefix for MLS_DForm2_r0.

jsji added inline comments.Jan 13 2020, 2:53 PM
llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
360

Do we have relocation similar to @l? Do we need to make this as ContextImmediate?

llvm/lib/Target/PowerPC/PPC.td
217

Shouldn't this be in https://reviews.llvm.org/D72574 instead?

llvm/lib/Target/PowerPC/PPCInstrFormats.td
45

As mentioned in comments in line 33 above, we should add the enum in PPCInstrInfo.h as well.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
35

It would be better to put these before TSFlags{8} so that they are in order and easier to check.

nemanjai added inline comments.Jan 13 2020, 5:10 PM
llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp
366

I think that according to Daniel's comment in https://reviews.llvm.org/D72574, we should be able to safely give Inst the type uint64_t in all paths and simply avoid having the template instantiated with a 32-bit integer altogether. This would allow us to not need the changes in TblGen.

NeHuang edited the summary of this revision. (Show Details)Jan 15 2020, 12:48 PM
NeHuang edited the summary of this revision. (Show Details)
kamaub added a subscriber: kamaub.EditedJan 15 2020, 2:00 PM

This patch looks ready to land (once the other comments are addressed), I only have two of NFC request that can be done pre-commit for readability.

llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp
335

nit: Is it possible to use that actually type here instead of auto? I get that its hard to define because it's a template within a template but as far as i can tell in this use it would be uint32_t (*ReadFunc) = this would make the code a more readable I think, selecting which function to use base on the target is more clear this way to me with this way.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
83

should this be let Inst{11} = PCRel; to match the definition in MLS_DForm_R_SI34_RTA5 since PI sets it to zero and only isPCRel sets it to 1

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

Commandeer this patch.

NeHuang updated this revision to Diff 238372.EditedJan 15 2020, 2:46 PM

Addressed Nemanja and Daniel's comment to give Inst type uint64_t

NeHuang marked 2 inline comments as done.Jan 15 2020, 2:49 PM

A few more comments need to be addressed for this to proceed.

llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
360

I believe that all pc-relative relocations have only a single possible interpretation so we probably don't need a ContextImmediate. However, once the ABI is finalized we might want to revisit/reevaluate that. Perhaps just a FIXME in the code noting that?

// Once the PC-Rel ABI is finalized, evaluate whether a 34-bit
// ContextImmediate is needed.
llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp
335

I think auto is preferable here as it does not restrict future changes to the interface of the functions themselves. For example, if at some point another parameter needed to be added with a default argument, hard-coding the function type here would cause compilation errors without a particularly good reason.

llvm/lib/Target/PowerPC/PPC.td
217

Agreed.

llvm/lib/Target/PowerPC/PPCInstrFormats.td
45

Yup!

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
35

+1

83

I think this one is explicitly not PC-Rel so that's why it's set to zero.

llvm/lib/Target/PowerPC/PPCSubtarget.h
109

This of course also needs to go into the other patch. Also, both of these need to be initialized (to false) in the .cpp file.

260

Same as above, please move to other patch.

nemanjai requested changes to this revision.Jan 16 2020, 6:26 AM

Forgot to select request changes.

This revision now requires changes to proceed.Jan 16 2020, 6:26 AM
NeHuang marked 8 inline comments as done.Jan 16 2020, 7:23 AM
NeHuang added inline comments.
llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
360

Will do.

llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp
335

+1

llvm/lib/Target/PowerPC/PPC.td
217

Will do.

llvm/lib/Target/PowerPC/PPCInstrFormats.td
45

Thanks @jsji, will do.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
35

Will do

83

Agree, PC-Rel is not used in PLI8 and PLI, should set zero here to avoid confusion.

llvm/lib/Target/PowerPC/PPCSubtarget.h
109

Will do.

260

Will do

amyk added a subscriber: amyk.Jan 16 2020, 3:45 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCInstrFormats.td
48

s/Inidicate/Indicate

llvm/lib/Target/PowerPC/PPCScheduleP9.td
42–43

Perhaps we should also update the comment to include PrefixInstrs.

NeHuang updated this revision to Diff 238911.Jan 17 2020, 3:34 PM

Addressed comments above.

NeHuang marked 16 inline comments as done.Jan 17 2020, 3:37 PM
nemanjai accepted this revision.Jan 21 2020, 3:06 AM

LGTM. Thanks for addressing the comments.

This revision is now accepted and ready to land.Jan 21 2020, 3:06 AM
jsji added a comment.Jan 21 2020, 7:13 AM

FYI. Needs rebase to pick up https://reviews.llvm.org/D72649. Thanks.

Yi-Hong.Lyu requested changes to this revision.Jan 21 2020, 10:02 AM
Yi-Hong.Lyu added a subscriber: Yi-Hong.Lyu.
Yi-Hong.Lyu added inline comments.
llvm/lib/Target/PowerPC/PPCInstrFormats.td
45

Do we really need this flag? I think it is the flag that should be removed with removing UseVSXReg flag work in https://reviews.llvm.org/D58685.

This revision now requires changes to proceed.Jan 21 2020, 10:02 AM
NeHuang updated this revision to Diff 239596.EditedJan 22 2020, 7:46 AM

Addressed review comments to pick up D72649 and remove the unused flag.
@Yi-Hong.Lyu

NeHuang marked an inline comment as done.Jan 22 2020, 7:49 AM
Yi-Hong.Lyu accepted this revision.Jan 22 2020, 12:04 PM
This revision is now accepted and ready to land.Jan 22 2020, 12:04 PM
Yi-Hong.Lyu requested changes to this revision.Jan 23 2020, 1:33 AM
Yi-Hong.Lyu added inline comments.
llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp
197

Two things:

  1. There is no test that execute this line. Tests doesn't have enough coverage in this patch.
  2. For paddi 1, 2, 8589934591, 1, it is disassemblable but invalid on hardware. I imagine we should use SoftFail instead of Fail here. Please see https://llvm.org/doxygen/classllvm_1_1MCDisassembler.html#a8eb822283e8f3200ca4b2a1ba0174e6a
This revision now requires changes to proceed.Jan 23 2020, 1:33 AM
nemanjai added inline comments.Jan 23 2020, 5:00 AM
llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp
197

I am not sure what your intent here is. The instruction as you wrote it is illegal. It will cause a SIGILL not some kind of undefined behaviour or anything like that.
What would be the advantage of disassembling object code into invalid instruction forms?

Yi-Hong.Lyu accepted this revision.Jan 23 2020, 7:00 AM
Yi-Hong.Lyu added inline comments.
llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp
197

Got it. Please add MC/Disassembler/PowerPC/future-invalid.txt in other pull request in this patch, otherwise this line is not tested. Other LGTM.

This revision is now accepted and ready to land.Jan 23 2020, 7:00 AM
This revision was automatically updated to reflect the committed changes.
NeHuang added a comment.EditedJan 24 2020, 5:55 AM

Thanks @Yi-Hong.Lyu @nemanjai. That test case was in https://reviews.llvm.org/D72574 and I have moved it to this patch.

NeHuang marked 3 inline comments as done.Jan 24 2020, 6:00 AM