[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

136 ↗

(On Diff #109088)

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

138 ↗

(On Diff #109088)

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

[spetrovic]

lib/Driver/ToolChains/Arch/ARM.cpp

136 ↗

(On Diff #109088)

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

[spetrovic]

ping

[kristof.beyls]

include/clang/Driver/Options.td

1664–1665 ↗

(On Diff #109571)

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

128 ↗

(On Diff #109571)

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

145–146 ↗

(On Diff #109571)

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

78–82 ↗

(On Diff #109571)

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

1664–1665 ↗

(On Diff #109571)

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

lib/Driver/ToolChains/Arch/ARM.cpp

128 ↗

(On Diff #109571)

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

145–146 ↗

(On Diff #109571)

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

78–82 ↗

(On Diff #109571)

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

145–146 ↗

(On Diff #109571)

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 ↗

(On Diff #114573)

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 ↗

(On Diff #114573)

a return ReadTPMode::Invalid is missing here.

[spetrovic]

lib/Driver/ToolChains/Arch/ARM.cpp

149–150 ↗

(On Diff #114573)

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.

cfe/trunk/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

cfe/trunk/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)">;

cfe/trunk/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=">;

cfe/trunk/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,

cfe/trunk/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) {
+  if (Arg *A = Args.getLastArg(options::OPT_mtp_mode_EQ)) {
+    const Driver &D = TC.getDriver();
+    arm::ReadTPMode 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::Invalid;
+  }
+  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 =
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);

cfe/trunk/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