Page MenuHomePhabricator

[PowerPC] Add assembly comments for instructions that use the vector registers.
AcceptedPublic

Authored by stefanp on Jan 6 2022, 7:08 AM.

Details

Reviewers
nemanjai
lei
amyk
Group Reviewers
Restricted Project
Summary

A number of register types overlap with the vector registers. When reading the
assembly it is not immediately obvious what vector registers overlap with what
other registers. This patch aims to add a comment to show which VSR registers
are used or defined for each instruction.

Diff Detail

Event Timeline

stefanp created this revision.Jan 6 2022, 7:08 AM
stefanp requested review of this revision.Jan 6 2022, 7:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2022, 7:08 AM
stefanp added a reviewer: Restricted Project.Jan 6 2022, 7:09 AM
amyk added a subscriber: amyk.Jan 7 2022, 6:59 AM

nit: The description has regsiter a number of times, please update it to register

This a ver useful patch. Thanks Stefan! I have a question (might be silly but...), if verbose asm is enabled by default, does it require an update to more than the two test cases that you've updated in this patch?
(I guess what I am getting at here is if verbose asm is enabled by default, I would have expected a whole bunch of other tests to also require updating, but I may be misunderstanding here.)

llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstComments.cpp
23
125
yinyuefengyi removed a subscriber: yinyuefengyi.
stefanp edited the summary of this revision. (Show Details)Jan 18 2022, 4:14 AM
stefanp updated this revision to Diff 400812.Jan 18 2022, 4:26 AM

Updated a couple of comments.

nit: The description has regsiter a number of times, please update it to register

This a ver useful patch. Thanks Stefan! I have a question (might be silly but...), if verbose asm is enabled by default, does it require an update to more than the two test cases that you've updated in this patch?
(I guess what I am getting at here is if verbose asm is enabled by default, I would have expected a whole bunch of other tests to also require updating, but I may be misunderstanding here.)

For some reason I always seem to typo the word register. Not sure why... Anyway, thank you for finding them.

Not a silly question at all!
Initially I thought that I didn't need to update the tests because I wan't changing code and the CHECK/CHECK-NEXT lines don't really care that there are comments that come after the instructions. This is true in most cases but there are tests that already have a comment after the instruction line (for example # 8-byte Folded Spill) and in those cases by adding a second comment I'm adding a new line and the CHECK-NEXT lines don't work anymore. Anyway, I'm going to update those tests now.

stefanp updated this revision to Diff 400954.Jan 18 2022, 12:37 PM

Updated a whole set of tests that now have additional comments.

amyk accepted this revision as: amyk.Jan 21 2022, 2:20 PM

nit: The description has regsiter a number of times, please update it to register

This a ver useful patch. Thanks Stefan! I have a question (might be silly but...), if verbose asm is enabled by default, does it require an update to more than the two test cases that you've updated in this patch?
(I guess what I am getting at here is if verbose asm is enabled by default, I would have expected a whole bunch of other tests to also require updating, but I may be misunderstanding here.)

For some reason I always seem to typo the word register. Not sure why... Anyway, thank you for finding them.

Not a silly question at all!
Initially I thought that I didn't need to update the tests because I wan't changing code and the CHECK/CHECK-NEXT lines don't really care that there are comments that come after the instructions. This is true in most cases but there are tests that already have a comment after the instruction line (for example # 8-byte Folded Spill) and in those cases by adding a second comment I'm adding a new line and the CHECK-NEXT lines don't work anymore. Anyway, I'm going to update those tests now.

Thanks for addressing the comment, Stefan!
I think overall, it LGTM.

This revision is now accepted and ready to land.Jan 21 2022, 2:20 PM
lei added inline comments.Feb 3 2022, 12:31 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstComments.cpp
33–40

This seems like something that can be useful in other implementations.
Wondering if it's better is we define functions such as isRegInClass(), isVecReg() similar to what's defined in PPCVSXSwapRemoval.cpp but have it as part of class PPCRegisterInfo. Currently I see diff implementation of similar checks in various classes.

113

Wondering why this is not implemented as part of the PPCInstPrinter class since it's only ever used for printing PPC instructions.

156

This bool return value seem to have no uses.

llvm/test/CodeGen/PowerPC/aix-p9-insert-extract.ll
152 ↗(On Diff #400954)

Seems Vec Uses: contain duplicate input for the same vector?

Vec Uses: V3(VSR35)V3(VSR35)
lei added a comment.Feb 3 2022, 12:35 PM

Not a silly question at all!
Initially I thought that I didn't need to update the tests because I wan't changing code and the CHECK/CHECK-NEXT lines don't really care that there are comments that come after the instructions. This is true in most cases but there are tests that already have a comment after the instruction line (for example # 8-byte Folded Spill) and in those cases by adding a second comment I'm adding a new line and the CHECK-NEXT lines don't work anymore. Anyway, I'm going to update those tests now.

Is it possible to append to the comment line instead of adding a new comment line below it? Not a huge deal but thought maybe it would look cleaner to have all comments on the same line. Or at least indented to indicate it's part of the previous line?