[PATCH] D59879: [ARM][CMSE] Add commandline option and feature macro

[Dave Green via Phabricator via llvm-commits]

dmgreen added reviewers: olista01, christof.
dmgreen added inline comments.


================
Comment at: include/clang/Driver/Options.td:2145
+  Flags<[DriverOption,CC1Option]>,
+  HelpText<"Allow use of CMSE instructions (ARM only)">;
 
----------------
Should this say something about compiling for secure state?


================
Comment at: lib/Basic/Targets/ARM.cpp:438
+    } else if (Feature == "+8msecext") {
+      if ((CPUProfile != "M" && CPUProfile != "B") || ArchVersion != 8) {
+        Diags.Report(diag::err_target_unsupported_mcmse) << CPU;
----------------
snidertm wrote:
> How does CPUProfile get a value of "B"? I thought any Cortex-M processor would set CPUProfile to "M". Is CMSE available on a processor besides Cortex-m33?
"B" was going to be a very old name for v8m-baseline, looks like this one was never cleaned up when that was changed. You can drop the != "B" check.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:1390
+
+  if (Args.getLastArg(options::OPT_mcmse)) {
+    CmdArgs.push_back("-mcmse");
----------------
Don't need the brackets here


================
Comment at: lib/Frontend/CompilerInvocation.cpp:2705
   Opts.GNUAsm = !Args.hasArg(OPT_fno_gnu_inline_asm);
+  Opts.Cmse = Args.hasArg(OPT_mcmse); // CMSE
 
----------------
This comment seems superfluous


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59879/new/

https://reviews.llvm.org/D59879