[PATCH] D34878: [ARM] Option for reading thread pointer from coprocessor register

[Strahinja Petrovic via Phabricator via cfe-commits]

spetrovic marked 3 inline comments as done.
spetrovic added inline comments.


================
Comment at: include/clang/Driver/Options.td:1664-1665
   HelpText<"Allow generation of data access to code sections (ARM only)">;
+def mtp_mode_EQ : Joined<["-"], "mtp=">, Group<m_arm_Features_Group>, Values<"soft, cp15">,
+  HelpText<"Read thread pointer from coprocessor register (ARM only)">;
 def mpure_code : Flag<["-"], "mpure-code">, Alias<mexecute_only>; // Alias for GCC compatibility
----------------
kristof.beyls wrote:
> Looking at the gcc documentation for this option (https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html), gcc accepts 3 values: 'soft', 'cp15' and 'auto', with the default setting being 'auto'.
> This patch implements just 2 of those values: 'soft' and 'cp15'.
> I think this is fine, as long as the default value is 'soft'.
> The 'auto' value should automatically pick 'cp15' if that's going to work on what you're targeting. If I understood correctly, that depends both on the architecture version you're targeting and the operating system/kernel you're targeting. So, there could be a lot of details to go through to get 'auto' right in all cases. Which is why I think it's fine to leave an implementation of 'auto' for later.
> Is the default value 'soft'?
I agree with your opinion about 'auto'. If -mtp option is not specified, yes, default value is soft.


================
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:128
+  const Driver &D = TC.getDriver();
+  arm::ReadTPMode ThreadPointer = ReadTPMode::Invalid;
+  if (Arg *A =
----------------
kristof.beyls wrote:
> Wouldn't it be better to default to ReadTPMode::Soft when not -mtp command line option is given? ....
When there is no -mtp in command line ReadTPMode::Soft is default value, ReadTPMode::Invalid is in case when someone try to put in -mtp value that is not cp15 or soft (e.g. -mtp=x).


================
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:145-146
+  // choose soft mode.
+  if (ThreadPointer == ReadTPMode::Invalid)
+    ThreadPointer = ReadTPMode::Soft;
+  return ThreadPointer;
----------------
kristof.beyls wrote:
> .... and always give an error if an invalid mtp command line option was given, rather than default back to soft mode?
If 'mtp' takes invalid value, error is always provided. This is the case when there is no -mtp option in command line, you can see how the case of invalid mtp argument is handled in the code above.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:1348-1358
+  arm::ReadTPMode ThreadPointer = arm::getReadTPMode(getToolChain(), Args);
+  if (ThreadPointer == arm::ReadTPMode::Cp15) {
+    CmdArgs.push_back("-mtp");
+    CmdArgs.push_back("cp15");
+  } else {
+    assert(ThreadPointer == arm::ReadTPMode::Soft &&
+           "Invalid mode for reading thread pointer");
----------------
kristof.beyls wrote:
> My inexperience in this part of the code base is probably showing, but why is this needed at all?
> IIUC, in D34408, you modelled TPMode in the backend using a target feature, and there isn't a custom -mtp option there?
> Maybe this is left-over code from an earlier revision of D34408, that's no longer needed?
Well, actually you are right, we can remove this part of the code, I was thinking that maybe someone in future will need in backend that '-mtp' option also can be recgonized, so I added this, but you are right, I will remove this.


================
Comment at: test/Driver/clang-translation.c:78-82
+// RUN: %clang -target arm-linux -mtp=cp15 -### -S %s -arch armv7 2>&1 | \
+// RUN: FileCheck -check-prefix=ARMv7_THREAD_POINTER %s
+// ARMv7_THREAD_POINTER: "-target-feature" "+read-tp-hard"
+// ARMv7_THREAD_POINTER: "-mtp" "cp15"
+// ARMv7_THREAD_POINTER-NOT: "mtp" "soft"
----------------
kristof.beyls wrote:
> It probably would be good to also have a test that when no mtp option is given, the equivalent of when '-mtp soft' is specified would happen.
> Furthermore, my inexperience in this part of the code base probably shows, but I'm puzzled as to why this test is looking for '-mtp' in the output. Wouldn't the '-target-feature +read-tp-hard' be enough to convey the information to the mid- and back-end?
Checks for '-mtp' in the output are unnecessary when we remove part of the code that you mentioned in previous comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D34878