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

Dave Green via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 14 12:04:08 PDT 2018


dmgreen added a comment.

In https://reviews.llvm.org/D49673#1198678, @peter.smith wrote:

> Generally looking good to me. Apologies for the delay.


Thanks for the review

> 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.

If in doubt, follow what was already there. I'll try to keep things as they were.

> It would be good to add a test in MC for a relocated ADR instruction with the :got: modifier.

Added some test for adr/adrp errors. There are others in MC to cover testing adr with a label. I changed the tryParseAdrLabel a little to make the error's better.

> It would also be good to add a test to check what happens when someone attempts to use TLS with the tiny code model.

Hmm.. So I was expecting this would keep working the same as small model. I believe that would be less efficient but functionally correct (providing you are not un-aligning page boundaries). In adding some tests I can see that there are some code differences though (because it goes through LOADgot). I think the example in arm64-tls-execs is correct, and the other tests here produce the same code (as does your example from above). I've not tested this very well though, and I'm not sure what else would be required. As far as I can tell it should be OK, but producing an error for tiny+tls, same as large+tls, is an option.



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:4073
+  assert((Subtarget->useSmallAddressing() ||
+          getTargetMachine().getCodeModel() == CodeModel::Tiny) &&
          "ELF TLS only supported in small memory model");
----------------
peter.smith wrote:
> 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.
I agree the behaviour here for Large isn't so great. It will assert, or just use the small code here in release builds without asserts. I've changed that to a fatal error.

I'm not sure how to add a warning for tiny+tls.


https://reviews.llvm.org/D49673





More information about the llvm-commits mailing list