This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][NFC] Rename instruction formats in PPCInstrPrefix.td
ClosedPublic

Authored by amyk on Mar 7 2020, 7:42 PM.

Details

Summary

This patch renames some of the instruction formats within PPCInstrPrefix.td to adopt a more uniform naming convention.

Diff Detail

Event Timeline

amyk created this revision.Mar 7 2020, 7:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2020, 7:42 PM
amyk marked an inline comment as done.Mar 9 2020, 4:11 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
46

Note that I've changed this format to have SI34_FRS5. I was originally going to change just D34 to SI34 but then the instruction format name would be MLS_DForm_R_SI34_RTA5. This would not be possible, as there already exists an instruction format with this name (below the current instruction format).

lei accepted this revision.Mar 10 2020, 6:41 AM

Just 1 minor name change. Otherwise LGTM.

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

Based on the other names, I think this should be MLS_DForm_R_SI34_FRSA5 since D_RA is 34 bits immediate + 5 bit register field RA.

This revision is now accepted and ready to land.Mar 10 2020, 6:41 AM
stefanp requested changes to this revision.Mar 11 2020, 1:05 PM

Talked this over with the team and we have decided on a way to rename that class. See the comment.

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

Hi Amy,
After a discussion with the team we think it would be better just to add a _MEM to this name (So, MLS_DForm_R_SI34_RTA5_MEM).

The two names are the same because the formats are effectively the same. This is just a special case where the immediate and the register have been combined to form a memory address. If we add the MEM at the end we can indicate that this is a memory op and make that the difference from the other class.

This revision now requires changes to proceed.Mar 11 2020, 1:05 PM
amyk updated this revision to Diff 249795.Mar 11 2020, 4:30 PM

Addressed review comment of adding _MEM to the instruction format.

amyk marked 2 inline comments as done.Mar 11 2020, 4:31 PM

@lei @stefanp I've changed the instruction format to accommodate _MEM in the naming. Please let me know if you have any other concerns.

lei added a comment.Mar 11 2020, 4:50 PM

Thanks for the update!

stefanp accepted this revision.Mar 12 2020, 10:34 AM

Thank you for fixing this Amy.
LGTM.

This revision is now accepted and ready to land.Mar 12 2020, 10:34 AM
This revision was automatically updated to reflect the committed changes.