This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Unify handling of M-Class system registers
ClosedPublic

Authored by javed.absar on Jul 10 2017, 10:04 AM.

Details

Summary

This patch cleans up and fixes issues in the M-Class system register handling:

  1. It defines the system registers and the encoding (SYSm values) in one place: ARMRegisterInfo.td, removing the hand-coded values which existed duplicated in multiple places - ARMISelDAGToDAG.cpp, ARMAsmParser.cpp and ARMInstPrinter.cpp.
  1. Some system registers e.g. BASEPRI_MAX_NS which do not exist (reference - ARMv6/7/8M architecture reference manual) were being allowed.

I have tried to solve those issues by clearly defining in one places which registers exist and in which mode.

Diff Detail

Event Timeline

javed.absar created this revision.Jul 10 2017, 10:04 AM
john.brawn edited edge metadata.Jul 11 2017, 3:41 AM

ARMISelDAGToDAG.cpp and ARMAsmParser.cpp have essentially duplicate code for mapping the register name to the register enum, instead they should both call a function to handle it. I'm not sure where it would go though, maybe ARMBaseRegisterInfo? It would also make sense to move the reverse mapping (done by ARMInstPrinter.cpp) there as well so you only need to modify one file when adding a new register.

Another option would be to table-generate this mapping using SearchableTable.td, as is currently done for AArch64 system registers and instructions. This would allow us to have one table in the source, from which both the name->encoding and encoding->name mappings are generated.

javed.absar added a reviewer: olista01.

I have re-implemented using SearchableTable which improves the code quality immensely (great suggestion by Oliver). I have also create a Utils sub-directory (and ARMUtils library) that allows utility functions such as generated by SearchableTable to be incorporated into both CodeGen and others. seamlessly.

olista01 added inline comments.Jul 14 2017, 5:49 AM
lib/Target/ARM/ARMISelDAGToDAG.cpp
3803–3804

These masks are duplicated in a few different places, would it be better to have a few functions in the ARM/Utils library to check if a register is valid, extract the encoding bits, etc?

t.p.northover added inline comments.
lib/Target/ARM/ARMSystemRegister.td
27–30

These seem to be duplicating the existing subtarget feature interface used by the assembler, which ought to be available (and used in the AArch64 sysreg definitions).

I could see using a single bit being neater for 1-2 such features, but it seems a little dubious for 4 of them. So is there any particular reason you dropped the "Requires" interface?

Thanks Tim/others for the suggestions. Updated, using 'Requires' , FeatureBits and factoring out common functions. Is much more compact and unified now, as I set out to do.

Ping - @t.p.northover @olista01 @john.brawn
I think all changes requested are done and the initial idea of getting everything in one place etc is met.

olista01 accepted this revision.Jul 18 2017, 8:36 AM

LGTM with a few minor changes.

lib/Target/ARM/ARMSystemRegister.td
18

This comment could be moved down next to the registers in question.

74

These comments can be removed as they are now obvious from the "let Requires =" lines.

This revision is now accepted and ready to land.Jul 18 2017, 8:36 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 23 2018, 6:25 AM

Sorry about the belated review comment, but:

llvm/trunk/lib/Target/ARM/Utils/ARMBaseInfo.h
23 ↗(On Diff #107292)

This line means that Utils depends on MCTargetDesc. But MCTargetDesc depends on InstPrinter, which depends on Util. So this gives us a logical dependency cycle. Can you fix this?