[NFC] Extract unifyTargetFeatures

[yaxunl]

Summary

extract unifyTargetFeatures to be used by lld.

[tra]

clang/lib/Driver/ToolChains/CommonArgs.cpp

93

I don't think assert should be used for something that may be specified to the user on command line.

97

Would it be simpler to just iterate over features backwards and skip the features we've already seen?

clang/lib/Driver/ToolChains/CommonArgs.h

120

Returning a new std::vector<StringRef> would make it more convenient to use and there's less complexity -- no need for the caller to create a temporary vector and the function itself can just return the temporary vector it collects the items in.

for (auto Feature : unifyTargetFeatures(Features)) {
  ...
}

[yaxunl]

clang/lib/Driver/ToolChains/CommonArgs.cpp

93

The entries in this array is not directly from command line.

It is either put in by clang code, or translated from -mxxx or -mno-xxx options whereas the translated entry should always have + or - prefix. So if things go wrong, it is due to developers.

97

That may save a little when there are repeating entries but you need to reverse the obtained vector to maintain the order. Since most cases there are just a few unique features, reversing may ends up slower.

[yaxunl]

return the unified features.

[tra]

clang/lib/Driver/ToolChains/CommonArgs.cpp

93

While clang may be the primary user of those options, features can be specified by the user, too.
The general rule of thumb is that invalid external input should produce diagnostics, not compiler crashes.
Asserts are for programming errors. Granted, when the inputs may be generated by our code the boundary is blurred, but in this case I would err on the side of not crashing.

97

Up to you. Performance does not matter here, but simpler code with improved readability is always a plus.

[yaxunl]

clang/lib/Driver/ToolChains/CommonArgs.cpp

93

user may use -target-feature to directly specify features and they may forget +/- there but that is a -cc1 option, whereas this function is used in tool and will never be used in -cc1. The features in this vector can only come from clang driver, where the code is supposed to always add +/- to the entries. How could user inputs can cause +/- not added?

[tra]

clang/lib/Driver/ToolChains/CommonArgs.cpp

93

Got it. I indeed had -target-feature in mind: https://github.com/llvm/llvm-project/blob/32ea3397bec820f98f0b77ca37244142ce700207/clang/lib/Frontend/CompilerInvocation.cpp#L3648

clang/lib/Driver/ToolChains/Clang.cpp

case llvm::Triple::amdgcn:
     break;
   case llvm::Triple::msp430:
     msp430::getMSP430TargetFeatures(D, Args, Features);
     break;
   case llvm::Triple::ve:
     ve::getVETargetFeatures(D, Args, Features);
   }
 
-  // Find the last of each feature.
-  llvm::StringMap<unsigned> LastOpt;
-  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
-    StringRef Name = Features[I];
-    assert(Name[0] == '-' || Name[0] == '+');
-    LastOpt[Name.drop_front(1)] = I;
-  }
-
-  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
-    // If this feature was overridden, ignore it.
-    StringRef Name = Features[I];
-    llvm::StringMap<unsigned>::iterator LastI = LastOpt.find(Name.drop_front(1));
-    assert(LastI != LastOpt.end());
-    unsigned Last = LastI->second;
-    if (Last != I)
-      continue;
-
+  for (auto Feature : unifyTargetFeatures(Features)) {
     CmdArgs.push_back(IsAux ? "-aux-target-feature" : "-target-feature");
-    CmdArgs.push_back(Name.data());
+    CmdArgs.push_back(Feature.data());
   }
 }
 
 static bool
 shouldUseExceptionTablesForObjCExceptions(const ObjCRuntime &runtime,
                                           const llvm::Triple &Triple) {
   // We use the zero-cost exception tables for Objective-C if the non-fragile
   // ABI is enabled or when compiling for x86_64 and ARM on Snow Leopard and

clang/lib/Driver/ToolChains/CommonArgs.h

 void AddTargetFeature(const llvm::opt::ArgList &Args,
                       std::vector<StringRef> &Features,
                       llvm::opt::OptSpecifier OnOpt,
                       llvm::opt::OptSpecifier OffOpt, StringRef FeatureName);
 
 std::string getCPUName(const llvm::opt::ArgList &Args, const llvm::Triple &T,
                        bool FromAs = false);
 
+/// Iterate \p Args and convert -mxxx to +xxx and -mno-xxx to -xxx and
+/// append it to \p Features.
+///
+/// Note: Since \p Features may contain default values before calling
+/// this function, or may be appended with entries to override arguments,
+/// entries in \p Features are not unique.
 void handleTargetFeaturesGroup(const llvm::opt::ArgList &Args,
                                std::vector<StringRef> &Features,
                                llvm::opt::OptSpecifier Group);
 
+/// If there are multiple +xxx or -xxx features, keep the last one.
+std::vector<StringRef>
+unifyTargetFeatures(const std::vector<StringRef> &Features);
+
 /// Handles the -save-stats option and returns the filename to save statistics
 /// to.
 SmallString<128> getStatsFileName(const llvm::opt::ArgList &Args,
                                   const InputInfo &Output,
                                   const InputInfo &Input, const Driver &D);
 
 /// \p Flag must be a flag accepted by the driver with its leading '-' removed,
 //     otherwise '-print-multi-lib' will not emit them correctly.

clang/lib/Driver/ToolChains/CommonArgs.cpp

for (const Arg *A : Args.filtered(Group)) {
 
     bool IsNegative = Name.startswith("no-");
     if (IsNegative)
       Name = Name.substr(3);
     Features.push_back(Args.MakeArgString((IsNegative ? "-" : "+") + Name));
   }
 }
 
+std::vector<StringRef>
+tools::unifyTargetFeatures(const std::vector<StringRef> &Features) {
+  std::vector<StringRef> UnifiedFeatures;
+  // Find the last of each feature.
+  llvm::StringMap<unsigned> LastOpt;
+  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
+    StringRef Name = Features[I];
+    assert(Name[0] == '-' || Name[0] == '+');
+    LastOpt[Name.drop_front(1)] = I;
+  }
+
+  for (unsigned I = 0, N = Features.size(); I < N; ++I) {
+    // If this feature was overridden, ignore it.
+    StringRef Name = Features[I];
+    llvm::StringMap<unsigned>::iterator LastI = LastOpt.find(Name.drop_front(1));
+    assert(LastI != LastOpt.end());
+    unsigned Last = LastI->second;
+    if (Last != I)
+      continue;
+
+    UnifiedFeatures.push_back(Name);
+  }
+  return UnifiedFeatures;
+}
+
 void tools::addDirectoryList(const ArgList &Args, ArgStringList &CmdArgs,
                              const char *ArgName, const char *EnvVar) {
   const char *DirList = ::getenv(EnvVar);
   bool CombinedArg = false;
 
   if (!DirList)
     return; // Nothing to do.
 

clang/test/Driver/hip-toolchain-features.hip

 // RUN:   2>&1 | FileCheck %s -check-prefix=ALL3
 // RUN: %clang -### -c -target x86_64-linux-gnu -fgpu-rdc \
 // RUN:   --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 %s \
 // RUN:   -mno-xnack -mno-sram-ecc \
 // RUN:   2>&1 | FileCheck %s -check-prefix=NOALL3
 
 // ALL3: {{.*}}clang{{.*}}"-target-feature" "+xnack" "-target-feature" "+sram-ecc"
 // NOALL3: {{.*}}clang{{.*}}"-target-feature" "-xnack" "-target-feature" "-sram-ecc"
+
+// RUN: %clang -### -target x86_64-linux-gnu -fgpu-rdc -nogpulib \
+// RUN:   --cuda-gpu-arch=gfx1010 %s \
+// RUN:   -mcumode -mcumode -mno-cumode -mwavefrontsize64 -mcumode \
+// RUN:   -mwavefrontsize64 -mno-wavefrontsize64 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=DUP
+// DUP: {{.*}}clang{{.*}} "-target-feature" "-wavefrontsize16"
+// DUP-SAME: "-target-feature" "+wavefrontsize32"
+// DUP-SAME: "-target-feature" "-wavefrontsize64"
+// DUP-SAME: "-target-feature" "+cumode"