[Target] Make a copy of TargetOptions feature list before sorting during CodeGen

[craig.topper]

Summary

Currently CodeGen is calling std::sort on the features vector in TargetOptions for every function, but I don't think CodeGen should be modifying TargetOptions.

This patch makes a copy before sorting.

[echristo]

This seems strictly more difficult to keep under control? Though I guess the assert helps.

Feel free to go ahead, but...

[craig.topper]

Yeah don't like it either, but was surprised to find that CodeGen even had a non-const reference to TargetOptions. Would it be better to make a copy and sort the copy during CodeGen?

[echristo]

No strong opinion. I think I like that one more though.

[craig.topper]

Make a copy before sorting and combine some repeated code that this exposed.

lib/Basic/Targets.cpp

TargetInfo::CreateTargetInfo(DiagnosticsEngine &Diags,
   // Add the features to the compile options.
   Opts->Features.clear();
   for (const auto &F : Features)
     Opts->Features.push_back((F.getValue() ? "+" : "-") + F.getKey().str());
 
   if (!Target->handleTargetFeatures(Opts->Features, Diags))
     return nullptr;
 
+  // Sort the features.
+  std::sort(Opts->Features.begin(), Opts->Features.end());
+
   Target->setSupportedOpenCLOpts();
   Target->setOpenCLExtensionOpts();
   Target->setMaxAtomicWidth();
 
   if (!Target->validateTarget(Diags))
     return nullptr;
 
   return Target.release();
 }

lib/Basic/Targets/AMDGPU.cpp

TargetOpts.Features.push_back(
         (Twine(hasFullSpeedFMAF32(TargetOpts.CPU) && !CGOpts.FlushDenorm
                    ? '+'
                    : '-') +
          Twine("fp32-denormals"))
             .str());
   // Always do not flush fp64 or fp16 denorms.
   if (!hasFP64Denormals && hasFP64)
     TargetOpts.Features.push_back("+fp64-fp16-denormals");
+
+  // Re-sort the features for CodeGen.
+  std::sort(TargetOpts.Features.begin(), TargetOpts.Features.end());
 }
 
 AMDGPUTargetInfo::GPUKind AMDGPUTargetInfo::parseR600Name(StringRef Name) {
   return llvm::StringSwitch<GPUKind>(Name)
       .Case("r600", GK_R600)
       .Case("rv610", GK_R600)
       .Case("rv620", GK_R600)
       .Case("rv630", GK_R600)

lib/CodeGen/CGCall.cpp

if (FD && FD->hasAttr<TargetAttr>()) {
         std::sort(Features.begin(), Features.end());
         FuncAttrs.addAttribute(
             "target-features",
             llvm::join(Features, ","));
       }
     } else {
       // Otherwise just add the existing target cpu and target features to the
       // function.
-      std::vector<std::string> &Features = getTarget().getTargetOpts().Features;
+      const std::vector<std::string> &Features =
+          getTarget().getTargetOpts().Features;
       if (TargetCPU != "")
         FuncAttrs.addAttribute("target-cpu", TargetCPU);
       if (!Features.empty()) {
-        std::sort(Features.begin(), Features.end());
+        assert(std::is_sorted(Features.begin(), Features.end()) &&
+               "Features not sorted?");
         FuncAttrs.addAttribute(
             "target-features",
             llvm::join(Features, ","));
       }
     }
   }
 
   ClangToLLVMArgMapping IRFunctionArgs(getContext(), FI);