[ARM64EC][clang-cl] Add arm64EC test; NFC

[efriedma]

Please add a testcase verifying the interaction between "/arm64EC" and explicitly specifying "--target". (If we end up ignoring the "/arm64EC" flag, we probably want a warning.)

[bcl5980]

add warning when /arm64EC has been override

[DavidSpickett]

Has Microsoft documented this option? Not doubting it exists but I mostly see how to use it rather than a specific page describing the option and its behaviour.

Not a big deal but if there is one, please include a link to it in the commit message.

clang/lib/Driver/Driver.cpp

1384

This would read better to me if you put them all in one and put the most important check first like:

if ( UArgs->hasArg(options::OPT__SLASH_arm64EC) &&
    ( (TC.getTriple().getArch() != llvm::Triple::aarch64 ||
      TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec)) {

clang/test/Driver/cl-options.c

787

Add an ARM64EC-NOT check for this part to check there is no warning.

Not going to catch much in this change but if someone goes digging into target setting they might change it by other means than --target and not realise.

790

A space after the ':' is a bit easier to parse IMO (unless this is matching an msvc warning in which case fair enough keep it as is).

[bcl5980]

address comments.

[bcl5980]

In D134788#3823181, @DavidSpickett wrote:

Has Microsoft documented this option? Not doubting it exists but I mostly see how to use it rather than a specific page describing the option and its behaviour.

Not a big deal but if there is one, please include a link to it in the commit message.

For now, I haven't found any document for this option. I can update later when Microsoft publish the information.

[DavidSpickett]

Please mark comments as done if you feel that they have been addressed. (I know this can be a bit weird, do you mark them or do I, I go with the former, I can disagree if needed)

Lack of docs right now is fine, we have enough to know this exists and it's quite simple.

With my one final comment addressed, this LGTM.

clang/lib/Driver/Driver.cpp

1384

This is good but I didn't emphasise one bit. Putting the arm64ec option check first saves reading the rest if the reader knows it's not relevant.

[bcl5980]

clang/lib/Driver/Driver.cpp

1384

I believe the condition UArgs->hasArg(options::OPT__SLASH_arm64EC) should be much heavier than
(TC.getTriple().getArch() != llvm::Triple::aarch64 || TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec).
So I put the Arch and SubArch check first here. I don't understand why we should put the hasArg check first.

[DavidSpickett]

This looks fine as is, all comments addressed as far as I can see.

clang/lib/Driver/Driver.cpp

1384

If the cost of the check is higher then this is fine keep it as is.

The reason that I personally would reorder them (if costs were equal) is this.

Imagine this didn't have 3 conditions it had 100.

if ((A or B or C or D or E...
     <this continues....>
    ) && some_option_used)

Why did I, the reader, have to parse 99 seemingly random conditions (which have to live in my short term memory) only to find that oh, this is all predicated on that one option being passed. If I don't actually care about that one option, I just wasted my time.

And ok, the average if doesn't have 100 conditions but if you keep this in mind even for smaller checks it adds up over time.

Code is going to be read more than it is written.

This applies to early returns too. Imagine I care about the !=10 case:

void foo(int i) {
  if (i == 10) {
    // 500 lines of code you don't care about 50% of the time
  } else {
    // what you were actually looking for
    return;
  }

That situation does happen a lot, and the same principle can be applied.

void foo(int i) {
  if (i != 10)
     return;

  // 500 lines of code you can read if you actually need to
}

[bcl5980]

Thanks for the detail explaination. I agree that if the cost is the same hasArg is better to move to the first.

[efriedma]

Missing testcase for the case where the warning is triggered?

[bcl5980]

In D134788#3830987, @efriedma wrote:

Missing testcase for the case where the warning is triggered?

Based on David's comment I remove the test case with specified target.

Not going to catch much in this change but if someone goes digging into target setting they might change it by other means than --target and not realise.

If you think that's necessary I can commit a NFC change to add it.

[efriedma]

I think we want to test both that the warning isn't triggered when --target isn't specified, and that the warning is triggered when --target is specified.

clang/test/Driver/cl-options.c

788

I think the ARM64EC-NOT might be in the wrong place? I think the warning would show up before the final command line, not after it.

[DavidSpickett]

This (and it's followup?) has been landed, right? Please close the revision if so.

clang/include/clang/Driver/Options.td

def _SLASH_openmp_experimental : CLFlag<"openmp:experimental">,
   HelpText<"Enable OpenMP support with experimental SIMD support">,
   Alias<fopenmp>;
 def _SLASH_tune : CLCompileJoined<"tune:">,
   HelpText<"Set CPU for optimization without affecting instruction set">,
   Alias<mtune_EQ>;
 def _SLASH_QIntel_jcc_erratum : CLFlag<"QIntel-jcc-erratum">,
   HelpText<"Align branches within 32-byte boundaries to mitigate the performance impact of the Intel JCC erratum.">,
   Alias<mbranches_within_32B_boundaries>;
+def _SLASH_arm64EC : CLFlag<"arm64EC">,
+  HelpText<"Set build target to arm64ec">;
 
 // Non-aliases:
 
 def _SLASH_arch : CLCompileJoined<"arch:">,
   HelpText<"Set architecture for code generation">;
 
 def _SLASH_M_Group : OptionGroup<"</M group>">, Group<cl_compile_Group>;
 def _SLASH_volatile_Group : OptionGroup<"</volatile group>">,

clang/lib/Driver/Driver.cpp

Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   // and getToolChain is const.
   if (IsCLMode()) {
     // clang-cl targets MSVC-style Win32.
     llvm::Triple T(TargetTriple);
     T.setOS(llvm::Triple::Win32);
     T.setVendor(llvm::Triple::PC);
     T.setEnvironment(llvm::Triple::MSVC);
     T.setObjectFormat(llvm::Triple::COFF);
+    if (Args.hasArg(options::OPT__SLASH_arm64EC))
+      T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);
     TargetTriple = T.str();
   } else if (IsDXCMode()) {
     // Build TargetTriple from target_profile option for clang-dxc.
     if (const Arg *A = Args.getLastArg(options::OPT_target_profile)) {
       StringRef TargetProfile = A->getValue();
       if (auto Triple =
               toolchains::HLSLToolChain::parseTargetProfile(TargetProfile))
         TargetTriple = *Triple;
Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
 
   // Owned by the host.
   const ToolChain &TC = getToolChain(
       *UArgs, computeTargetTriple(*this, TargetTriple, *UArgs));
 
   // The compilation takes ownership of Args.
   Compilation *C = new Compilation(*this, TC, UArgs.release(), TranslatedArgs,
                                    ContainsError);
 
   if (!HandleImmediateArgs(*C))
     return C;
 
   // Construct the list of inputs.
   InputList Inputs;
   BuildInputs(C->getDefaultToolChain(), *TranslatedArgs, Inputs);
 
   // Populate the tool chains for the offloading devices, if any.

clang/test/Driver/cl-options.c

 // RUN: %clang_cl /external:W0 /c -### -- %s 2>&1 | FileCheck -check-prefix=EXTERNAL_W0 %s
 // RUN: %clang_cl /external:W1 /c -### -- %s 2>&1 | FileCheck -check-prefix=EXTERNAL_Wn %s
 // RUN: %clang_cl /external:W2 /c -### -- %s 2>&1 | FileCheck -check-prefix=EXTERNAL_Wn %s
 // RUN: %clang_cl /external:W3 /c -### -- %s 2>&1 | FileCheck -check-prefix=EXTERNAL_Wn %s
 // RUN: %clang_cl /external:W4 /c -### -- %s 2>&1 | FileCheck -check-prefix=EXTERNAL_Wn %s
 // EXTERNAL_W0: "-Wno-system-headers"
 // EXTERNAL_Wn: "-Wsystem-headers"
 
+// RUN: %clang_cl -vctoolsdir "" /arm64EC /c -### -- %s 2>&1 | FileCheck %s --check-prefix ARM64EC
+// ARM64EC: "-triple" "arm64ec-pc-windows-msvc19.20.0"
+
 void f(void) { }