[PATCH] D49673: [AArch64] Add Tiny Code Model for AArch64

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Generally looking good to me. Apologies for the delay.

As a general nit, the patch has been a bit inconsistent about preserving/removing curly braces after if/else statements with only a single statement in the body. It may be better to either preserve all as they are or change all of them; not a critical comment by any means.

It would be good to add a test in MC for a relocated ADR instruction with the :got: modifier. It would also be good to add a test to check what happens when someone attempts to use TLS with the tiny code model.



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:4073
+  assert((Subtarget->useSmallAddressing() ||
+          getTargetMachine().getCodeModel() == CodeModel::Tiny) &&
          "ELF TLS only supported in small memory model");
----------------
I think that this should be a user-visible error message as it is not obvious to a user why TLS might not be supported.

Another possibility is to just warn and use the small code model for TLS. This would cause problems for people that must avoid ADRP due to position independent requirements on 4k page alignment, but I think that could be a small minority of people.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:4795
+  if (!Subtarget->isTargetMachO()) {
+    if (getTargetMachine().getCodeModel() == CodeModel::Large)
+      return getAddrLarge(JT, DAG);
----------------
You may be able to simplify this as getTargetMachine().getCodeModel() == CodeModel::Tiny implies ELF as getEffectiveCodeModel so you shouldn't need to test if the Subtarget->isTargetMachO(). Probably not a lot in it though.


https://reviews.llvm.org/D49673