This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Tidy-up ARMAsmParser. NFC.
ClosedPublic

Authored by javed.absar on Aug 24 2017, 1:17 PM.

Details

Summary

Move getDRegFromQReg generic function from ARMAsmParser to Utils where it belongs.

Diff Detail

Event Timeline

javed.absar created this revision.Aug 24 2017, 1:17 PM
javed.absar retitled this revision from [ARM] Tidy-up ARMAsmParser to [ARM] Tidy-up ARMAsmParser. NFC..Aug 24 2017, 8:09 PM
asb added a subscriber: asb.Aug 25 2017, 7:00 AM

Why not use MCRegisterInfo::getSubReg for this purpose, thus avoiding repeating information that's already present in ARMRegisterInfo.td? MRI->getSubReg(QReg, ARM::dsub_0) should replace the functionality of getDRegFromQReg.

In D37118#852522, @asb wrote:

Why not use MCRegisterInfo::getSubReg for this purpose, thus avoiding repeating information that's already present in ARMRegisterInfo.td? MRI->getSubReg(QReg, ARM::dsub_0) should replace the functionality of getDRegFromQReg.

That's a good idea. Have done that instead. I don't see other uses of this function otherwise moving it to Utils as earlier would have been better i think.

fhahn edited edge metadata.Aug 25 2017, 10:54 AM

Would it be possible to just used MRI->getSubReg(QReg, ARM::dsub_0) wherever getDRegFromQReg is called? It does not seem like having a getDRegFromQReg function adds much value now.

Would it be possible to just used MRI->getSubReg(QReg, ARM::dsub_0) wherever getDRegFromQReg is called? It does not seem like having a getDRegFromQReg function adds much value now.

I count 8 instance of it being called, so woulld really prefer a separate function for clarity and let the compiler inline it.

asb added a comment.Aug 25 2017, 1:13 PM

Looks good to me. I agree it seems almost unnecessary to have a helper for MRI->getSubReg(QReg, ARM::dsub_0), but as Javed points out it's called many times and getDRegFromQReg is much more readable.

fhahn accepted this revision.Aug 27 2017, 3:26 AM

LGTM, using MRI->getSubReg seems much better! Thanks @asb for pointing that out, I assume there is still more potential for re-using more general functions in the ARM backend

lib/Target/ARM/AsmParser/ARMAsmParser.cpp
268

this could be a const function I think?

This revision is now accepted and ready to land.Aug 27 2017, 3:26 AM
javed.absar added inline comments.Aug 27 2017, 6:03 AM
lib/Target/ARM/AsmParser/ARMAsmParser.cpp
268

Yes indeed. Will do so before committing

This revision was automatically updated to reflect the committed changes.