[ARM] Option for reading thread pointer from coprocessor register

[spetrovic]

Summary

This patch enables option for reading thread pointer directly from coprocessor register (-mread-tp-hard). This option is supported in llc also ( https://reviews.llvm.org/D34408 ).

[spetrovic]

ping

[spetrovic]

ping

[bruno]

Hi Strahinja,

Does GCC already has a similar option? If so, does this share the same name?

[spetrovic]

Hi Bruno,

Yes, GCC has similar option (-mtp=soft/hard), but name is not same. I put the same option name as in backend (https://reviews.llvm.org/D34408).

[bruno]

If there's a precedence for a flag name there, it's nice to be compatible here. You don't necessarily need to use the same name as the backend.

[spetrovic]

Comments addressed.

[bruno]

lib/Driver/ToolChains/Arch/ARM.cpp

145

What happens if you pass an empty "-mtp=" to the driver? Will it silently assume soft? Shouldn't it be an error too?

147

Won't this assignment be covered by the code below anyway? Maybe remove it?

[spetrovic]

lib/Driver/ToolChains/Arch/ARM.cpp

145

I agree, it should be an error, I fixed that.

[spetrovic]

ping

[kristof.beyls]

include/clang/Driver/Options.td

1692–1693

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'?

lib/Driver/ToolChains/Arch/ARM.cpp

137

Wouldn't it be better to default to ReadTPMode::Soft when not -mtp command line option is given? ....

154–155

.... and always give an error if an invalid mtp command line option was given, rather than default back to soft mode?

lib/Driver/ToolChains/Clang.cpp

1348–1358 ↗

(On Diff #109571)

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?

test/Driver/clang-translation.c

90–94

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?

[spetrovic]

include/clang/Driver/Options.td

1692–1693

I agree with your opinion about 'auto'. If -mtp option is not specified, yes, default value is soft.

lib/Driver/ToolChains/Arch/ARM.cpp

137

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

154–155

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.

lib/Driver/ToolChains/Clang.cpp

1348–1358 ↗

(On Diff #109571)

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.

test/Driver/clang-translation.c

90–94

Checks for '-mtp' in the output are unnecessary when we remove part of the code that you mentioned in previous comment.

[kristof.beyls]

lib/Driver/ToolChains/Arch/ARM.cpp

154–155

Right.
I just thought that the function would be ever so slightly simpler if it had the following structure roughly:

if (Arg *A = ...) {
  ThreadPointer = llvm::StringSwitch... ;
  if (!Invalid)
    return ThreadPointer;
  if (empty)
    D.Diag();
  else
    D.Diag();
  return Invalid;
}
return ReadTPMode::Soft;

And probably is also slightly closer to the coding standard described in https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
But this is a really minor comment.

[spetrovic]

Comments addressed.

[kristof.beyls]

lib/Driver/ToolChains/Arch/ARM.cpp

137

With the new version of the code, there's no need to have a ThreadPointer variable declared here; it can be declared inside the if statement below instead.

149–150

a return ReadTPMode::Invalid is missing here.

[spetrovic]

lib/Driver/ToolChains/Arch/ARM.cpp

149–150

Yes, my mistake, I will fix that.

[kristof.beyls]

Thanks Strahinja!
I thought that some indentations looked a bit strange, so I'd just still check that clang-format formats your changes the same way.
Otherwise LGTM!

[spetrovic]

Thanks for the review! I will check indentations with clang format.

[spetrovic]

Indentations fixed.

[kristof.beyls]

Still LGTM; please commit.

include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_compilationdatabase : Error<
   "compilation database '%0' could not be opened: %1">;
 def err_drv_command_signalled : Error<
   "%0 command failed due to signal (use -v to see invocation)">;
 def err_drv_force_crash : Error<
   "failing because %select{environment variable 'FORCE_CLANG_DIAGNOSTICS_CRASH' is set|'-gen-reproducer' is used}0">;
 def err_drv_invalid_mfloat_abi : Error<
   "invalid float ABI '%0'">;
+def err_drv_invalid_mtp : Error<
+  "invalid thread pointer reading mode '%0'">;
+def err_drv_missing_arg_mtp : Error<
+  "missing argument to '%0'">;
 def err_drv_invalid_libcxx_deployment : Error<
   "invalid deployment target for -stdlib=libc++ (requires %0 or later)">;
 def err_drv_invalid_argument_to_fdebug_prefix_map : Error<
   "invalid argument '%0' to -fdebug-prefix-map">;
 def err_drv_malformed_sanitizer_blacklist : Error<
   "malformed sanitizer blacklist: '%0'">;
 
 def err_target_unsupported_arch

include/clang/Driver/CC1Options.td

def menable_no_infinities : Flag<["-"], "menable-no-infs">,
   HelpText<"Allow optimization to assume there are no infinities.">;
 def menable_no_nans : Flag<["-"], "menable-no-nans">,
   HelpText<"Allow optimization to assume there are no NaNs.">;
 def menable_unsafe_fp_math : Flag<["-"], "menable-unsafe-fp-math">,
   HelpText<"Allow unsafe floating-point math optimizations which may decrease "
            "precision">;
 def mfloat_abi : Separate<["-"], "mfloat-abi">,
   HelpText<"The float ABI to use">;
+def mtp : Separate<["-"], "mtp">,
+  HelpText<"Mode for reading thread pointer">;
 def mlimit_float_precision : Separate<["-"], "mlimit-float-precision">,
   HelpText<"Limit float precision to the given value">;
 def split_stacks : Flag<["-"], "split-stacks">,
   HelpText<"Try to use a split stack if possible.">;
 def mno_zero_initialized_in_bss : Flag<["-"], "mno-zero-initialized-in-bss">,
   HelpText<"Do not put zero initialized data in the BSS">;
 def backend_option : Separate<["-"], "backend-option">,
   HelpText<"Additional arguments to forward to LLVM backend (during code gen)">;

include/clang/Driver/Options.td

 def mlong_calls : Flag<["-"], "mlong-calls">, Group<m_Group>,
   HelpText<"Generate branches with extended addressability, usually via indirect jumps.">;
 def mno_long_calls : Flag<["-"], "mno-long-calls">, Group<m_Group>,
   HelpText<"Restore the default behaviour of not generating long calls">;
 def mexecute_only : Flag<["-"], "mexecute-only">, Group<m_arm_Features_Group>,
   HelpText<"Disallow generation of data access to code sections (ARM only)">;
 def mno_execute_only : Flag<["-"], "mno-execute-only">, Group<m_arm_Features_Group>,
   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
 def mno_pure_code : Flag<["-"], "mno-pure-code">, Alias<mno_execute_only>;
 def mtvos_version_min_EQ : Joined<["-"], "mtvos-version-min=">, Group<m_Group>;
 def mappletvos_version_min_EQ : Joined<["-"], "mappletvos-version-min=">, Alias<mtvos_version_min_EQ>;
 def mtvos_simulator_version_min_EQ : Joined<["-"], "mtvos-simulator-version-min=">;
 def mappletvsimulator_version_min_EQ : Joined<["-"], "mappletvsimulator-version-min=">, Alias<mtvos_simulator_version_min_EQ>;
 def mwatchos_version_min_EQ : Joined<["-"], "mwatchos-version-min=">, Group<m_Group>;
 def mwatchos_simulator_version_min_EQ : Joined<["-"], "mwatchos-simulator-version-min=">;

lib/Driver/ToolChains/Arch/ARM.h

 const std::string getARMArch(llvm::StringRef Arch, const llvm::Triple &Triple);
 StringRef getARMCPUForMArch(llvm::StringRef Arch, const llvm::Triple &Triple);
 StringRef getLLVMArchSuffixForARM(llvm::StringRef CPU, llvm::StringRef Arch,
                                   const llvm::Triple &Triple);
 
 void appendEBLinkFlags(const llvm::opt::ArgList &Args,
                        llvm::opt::ArgStringList &CmdArgs,
                        const llvm::Triple &Triple);
+enum class ReadTPMode {
+  Invalid,
+  Soft,
+  Cp15,
+};
+
 enum class FloatABI {
   Invalid,
   Soft,
   SoftFP,
   Hard,
 };
 
 FloatABI getARMFloatABI(const ToolChain &TC, const llvm::opt::ArgList &Args);
+ReadTPMode getReadTPMode(const ToolChain &TC, const llvm::opt::ArgList &Args);
 
 bool useAAPCSForMachO(const llvm::Triple &T);
 void getARMArchCPUFromArgs(const llvm::opt::ArgList &Args,
                            llvm::StringRef &Arch, llvm::StringRef &CPU,
                            bool FromAs = false);
 void getARMTargetFeatures(const ToolChain &TC, const llvm::Triple &Triple,
                           const llvm::opt::ArgList &Args,
                           llvm::opt::ArgStringList &CmdArgs,

lib/Driver/ToolChains/Arch/ARM.cpp

 
 bool arm::useAAPCSForMachO(const llvm::Triple &T) {
   // The backend is hardwired to assume AAPCS for M-class processors, ensure
   // the frontend matches that.
   return T.getEnvironment() == llvm::Triple::EABI ||
          T.getOS() == llvm::Triple::UnknownOS || isARMMProfile(T);
 }
 
+// Select mode for reading thread pointer (-mtp=soft/cp15).
+arm::ReadTPMode arm::getReadTPMode(const ToolChain &TC, const ArgList &Args) {
+  const Driver &D = TC.getDriver();
+  arm::ReadTPMode ThreadPointer = ReadTPMode::Invalid;
+  if (Arg *A =
+          Args.getLastArg(options::OPT_mtp_mode_EQ)) {
+    ThreadPointer = llvm::StringSwitch<arm::ReadTPMode>(A->getValue())
+                        .Case("cp15", ReadTPMode::Cp15)
+                        .Case("soft", ReadTPMode::Soft)
+                        .Default(ReadTPMode::Invalid);
+    if (ThreadPointer != ReadTPMode::Invalid)
+      return ThreadPointer;
+    if (StringRef(A->getValue()).empty())
+      D.Diag(diag::err_drv_missing_arg_mtp) << A->getAsString(Args);
+    else
+      D.Diag(diag::err_drv_invalid_mtp) << A->getAsString(Args);
+  }
+  return ReadTPMode::Soft;
+}
+
 // Select the float ABI as determined by -msoft-float, -mhard-float, and
 // -mfloat-abi=.
 arm::FloatABI arm::getARMFloatABI(const ToolChain &TC, const ArgList &Args) {
   const Driver &D = TC.getDriver();
   const llvm::Triple &Triple = TC.getEffectiveTriple();
   auto SubArch = getARMSubArchVersionNumber(Triple);
   arm::FloatABI ABI = FloatABI::Invalid;
   if (Arg *A =
           Args.getLastArg(options::OPT_msoft_float, options::OPT_mhard_float,
                           options::OPT_mfloat_abi_EQ)) {
void arm::getARMTargetFeatures(const ToolChain &TC,
                                ArgStringList &CmdArgs,
                                std::vector<StringRef> &Features,
                                bool ForAS) {
   const Driver &D = TC.getDriver();
 
   bool KernelOrKext =
       Args.hasArg(options::OPT_mkernel, options::OPT_fapple_kext);
   arm::FloatABI ABI = arm::getARMFloatABI(TC, Args);
+  arm::ReadTPMode ThreadPointer = arm::getReadTPMode(TC, Args);
   const Arg *WaCPU = nullptr, *WaFPU = nullptr;
   const Arg *WaHDiv = nullptr, *WaArch = nullptr;
 
   if (!ForAS) {
     // FIXME: Note, this is a hack, the LLVM backend doesn't actually use these
     // yet (it uses the -mfloat-abi and -msoft-float options), and it is
     // stripped out by the ARM target. We should probably pass this a new
     // -target-option, which is handled by the -cc1/-cc1as invocation.
for (const Arg *A :
       } else if (Value.startswith("-mhwdiv=")) {
         WaHDiv = A;
       } else if (Value.startswith("-march=")) {
         WaArch = A;
       }
     }
   }
 
+
+  if (ThreadPointer == arm::ReadTPMode::Cp15)
+    Features.push_back("+read-tp-hard");
+
   // Check -march. ClangAs gives preference to -Wa,-march=.
   const Arg *ArchArg = Args.getLastArg(options::OPT_march_EQ);
   StringRef ArchName;
   if (WaArch) {
     if (ArchArg)
       D.Diag(clang::diag::warn_drv_unused_argument)
           << ArchArg->getAsString(Args);
     ArchName = StringRef(WaArch->getValue()).substr(7);

test/Driver/clang-translation.c

 // ARMV7K-APPLE: -munwind-table
 
 // RUN: %clang -target arm-linux -### -S %s -march=armv5e 2>&1 | \
 // RUN: FileCheck -check-prefix=ARMV5E %s
 // ARMV5E: clang
 // ARMV5E: "-cc1"
 // ARMV5E: "-target-cpu" "arm1022e"
 
+// RUN: %clang -target arm-linux -mtp=cp15 -### -S %s -arch armv7 2>&1 | \
+// RUN: FileCheck -check-prefix=ARMv7_THREAD_POINTER-HARD %s
+// ARMv7_THREAD_POINTER-HARD: "-target-feature" "+read-tp-hard"
+
+// RUN: %clang -target arm-linux -mtp=soft -### -S %s -arch armv7 2>&1 | \
+// RUN: FileCheck -check-prefix=ARMv7_THREAD_POINTER_SOFT %s
+// ARMv7_THREAD_POINTER_SOFT-NOT: "-target-feature" "+read-tp-hard"
+
+// RUN: %clang -target arm-linux -### -S %s -arch armv7 2>&1 | \
+// RUN: FileCheck -check-prefix=ARMv7_THREAD_POINTER_NON %s
+// ARMv7_THREAD_POINTER_NON-NOT: "-target-feature" "+read-tp-hard"
+
 // RUN: %clang -target powerpc64-unknown-linux-gnu \
 // RUN: -### -S %s -mcpu=G5 2>&1 | FileCheck -check-prefix=PPCG5 %s
 // PPCG5: clang
 // PPCG5: "-cc1"
 // PPCG5: "-target-cpu" "g5"
 
 // RUN: %clang -target powerpc64-unknown-linux-gnu \
 // RUN: -### -S %s -mcpu=power7 2>&1 | FileCheck -check-prefix=PPCPWR7 %s