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

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls 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
----------------
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'?


================
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:128
+  const Driver &D = TC.getDriver();
+  arm::ReadTPMode ThreadPointer = ReadTPMode::Invalid;
+  if (Arg *A =
----------------
Wouldn't it be better to default to ReadTPMode::Soft when not -mtp command line option is given? ....


================
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:145-146
+  // choose soft mode.
+  if (ThreadPointer == ReadTPMode::Invalid)
+    ThreadPointer = ReadTPMode::Soft;
+  return ThreadPointer;
----------------
.... and always give an error if an invalid mtp command line option was given, rather than default back to soft mode?


================
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");
----------------
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?


================
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"
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D34878