This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Tidy up banked registers encoding
ClosedPublic

Authored by javed.absar on Aug 2 2017, 6:51 AM.

Details

Summary

Moves encoding (SYSm) information of banked registers to ARMSystemRegister.td, where it rightly belongs and forms a single point of reference in the code.

Diff Detail

Repository
rL LLVM

Event Timeline

javed.absar created this revision.Aug 2 2017, 6:51 AM
fhahn accepted this revision.Aug 2 2017, 6:59 AM

LGTM, looks like a straight-forward improvement. I just have 2 nits. Please hold off with committing for a bit, so other people have time to voice concerns.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp
4178 ↗(On Diff #109335)

Should this comment be moved to lib/Target/ARM/ARMSystemRegister.td too?

lib/Target/ARM/Utils/ARMBaseInfo.cpp
46 ↗(On Diff #109335)

not sure what the correct indentation for macros is, but this seems slightly inconsistent with lib/Target/ARM/Utils/ARMBaseInfo.h:69, where it was aligned with the code.

This revision is now accepted and ready to land.Aug 2 2017, 6:59 AM
rovka added inline comments.Aug 2 2017, 7:34 AM
lib/Target/ARM/Utils/ARMBaseInfo.cpp
46 ↗(On Diff #109335)

It's consistent with the indentation above, for GET_MYCLASSSYSTREG_IMPL, so if you change one you should change both.
While we're discussing indentation, I think we don't usually indent a namespace within a namespace, so both ARMSysReg and ARMBankedReg should be unindented here (luckily, the code in ARMSysReg isn't indented, so it would be a tiny fix).

javed.absar added inline comments.Aug 2 2017, 8:28 AM
lib/Target/ARM/AsmParser/ARMAsmParser.cpp
4178 ↗(On Diff #109335)

Yes. That should be. Thanks for catching it.

lib/Target/ARM/Utils/ARMBaseInfo.cpp
46 ↗(On Diff #109335)

I too was confused with what the right indentation should be.
I saw that Tim indented namespace within namespace (https://github.com/llvm-mirror/llvm/blob/master/lib/Target/AArch64/Utils/AArch64BaseInfo.cpp) so followed that one.

We also have the reverse mapping (encoding->name) in ARMInstPrinter::printBankedRegOperand, could that use this table too?

In this update-

  1. Removed the manual (name->Encoding) in ARMISelDAGToDAG. Had missed it out earlier.
  2. Moved the comment to ARMSystemRegister.td
  3. Fixed indentation of "namespace namespace " . I too think no indentation on namespace is right choice (as Diana also mentioned).

We also have the reverse mapping (encoding->name) in ARMInstPrinter::printBankedRegOperand, could that use this table too?

I will attempt to clean this in a separate patch as the existing handling seems bit messy/tricky.

fhahn added a comment.Aug 2 2017, 10:26 AM

Thanks, I'm happy with the indentation and lib/Target/ARM/ARMISelDAGToDAG.cpp was a good spot.

Thanks @fhahn . I will rebase and commit this patch now.
Will work on the reverse mapping which @olista01 pointed out after that.

This revision was automatically updated to reflect the committed changes.