[PATCH] D36219: [ARM] Tidy up banked registers encoding

[Javed Absar via Phabricator via llvm-commits]

javed.absar added inline comments.


================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:4178
 
   // The values here come from B9.2.3 of the ARM ARM, where bits 4-0 are SysM
   // and bit 5 is R.
----------------
fhahn wrote:
> Should this comment be moved to lib/Target/ARM/ARMSystemRegister.td too?
Yes. That should be. Thanks for catching it.


================
Comment at: lib/Target/ARM/Utils/ARMBaseInfo.cpp:46
+  namespace ARMBankedReg {
+#define GET_BANKEDREG_IMPL
+#include "ARMGenSystemRegister.inc"
----------------
rovka wrote:
> fhahn wrote:
> > 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.
> 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).
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. 



https://reviews.llvm.org/D36219