Page MenuHomePhabricator

[PowerPC] [NFC] Refactor code for printing register operands
ClosedPublic

Authored by nemanjai on Sep 25 2018, 6:47 AM.

Details

Summary

We have an unfortunate situation in our back end where we have to keep pairs of functions synchronized. Needless to say that this is not an ideal situation as it is very difficult to enforce. Even without bugs, it's annoying to have to do the same thing in two places.

This patch just refactors the code so that the two pairs of those functions that pertain to printing register operands are unified:

  • stripRegisterPrefix() - this just removes the letter prefixes from registers for the InstrPrinter and AsmPrinter. This patch provides this as a static member of PPCRegisterInfo
  • Handling of PPCII::UseVSXReg - there are 3 places where we do something special for instructions with that flag set. Each of those places does its own checking of this flag and implements code customization. Any changes to how we print/encode VSX/VMX registers require modifying all 3 places. This patch unifies this into a static function in PPCInstrInfo that returns the register number adjusted as needed.

I was going to commit this NFC patch without a review, but it's a somewhat significant refactoring so I thought I'd put it up to see if anyone objects first.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Sep 25 2018, 6:47 AM
nemanjai added a reviewer: syzaara.
inouehrs added inline comments.
lib/Target/PowerPC/PPCInstrInfo.h
415 ↗(On Diff #166871)

Why do we need OpNo? There is no use of this parameter.

jsji added inline comments.Sep 25 2018, 9:07 AM
lib/Target/PowerPC/PPCAsmPrinter.cpp
282 ↗(On Diff #166871)

Patch not up to date? Need to also update code from https://reviews.llvm.org/D52244.

nemanjai added inline comments.Sep 25 2018, 8:04 PM
lib/Target/PowerPC/PPCAsmPrinter.cpp
282 ↗(On Diff #166871)

I'll certainly rebase it on top of that patch, but I am not really modifying this code.

lib/Target/PowerPC/PPCInstrInfo.h
415 ↗(On Diff #166871)

There is currently no use for this parameter. But various pseudo-instructions we may need to add in the future may have different numbering schemes for different operands.

For example, I could implement something like this:
mypseudo VRT, XA, VRA which would do something on vector registers and XA is a 6-bit VSX register number, whereas VRT, VRA are 5-bit VMX register numbers.

inouehrs added inline comments.Sep 25 2018, 8:10 PM
lib/Target/PowerPC/PPCInstrInfo.h
415 ↗(On Diff #166871)

I see. Maybe it is nice if you can write so in comment.

nemanjai added inline comments.Sep 26 2018, 3:24 AM
lib/Target/PowerPC/PPCInstrInfo.h
415 ↗(On Diff #166871)

Will do. Thanks for the suggestion.

nemanjai updated this revision to Diff 167106.Sep 26 2018, 4:31 AM

Rebased and added requested comment.

If there are no further objections from @jsji or @inouehrs, please approve so I can commit this NFC patch.

jsji accepted this revision.Sep 26 2018, 7:55 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Sep 26 2018, 7:55 AM
This revision was automatically updated to reflect the committed changes.