[NVPTX] Add setAuxTarget override rather than make a new TargetInfo

[rprichard]

Summary

Previously, NVPTXTargetInfo constructed a TargetInfo using the
TargetOptions::HostTriple field, but did not initialize the target CPU
or CPU features, which resulted in an inaccurate value for
MaxAtomicInlineWidth on x86_32. MaxAtomicInlineWidth is 64 if the host
TargetInfo is initialized with the +cx8 feature (e.g. 586 and up) but
is 32 otherwise.

Instead, add a setAuxTarget override and defer copying from the host
TargetInfo until CompilerInstance::createTarget calls it.

Change setAuxTarget to pass IntrusiveRefCntPtr<TargetInfo> instead of
a const TargetInfo*. NVPTXTargetInfo needs to retain the host
TargetInfo so that it can check calling conventions.

NVPTXTargetInfo was the only user of the TargetOptions::HostTriple
field, so remove the field.

Also see D29542 and D56318.

[rprichard]

This change is needed for D28213, so that the __GCC_ATOMIC_LLONG_LOCK_FREE macro matches for -target i386-unknown-linux-gnu between --cuda-host-only and --cuda-device-only. This is tested in clang/test/Preprocessor/cuda-types.cu.

[yaxunl]

need a test

[rprichard]

I don't expect this change to affect the compiler behavior by itself -- is there a particular test that should be written?

The code that I'm moving into NVPTXTargetInfo::setAuxTarget is already tested via clang/test/Preprocessor/cuda-types.cu.

Aside: There are only two calls to AllocateTarget: one in TargetInfo::CreateTargetInfo and one in NVPTXTargetInfo::NVPTXTargetInfo. This change removes NVPTXTargetInfo's call.

[rprichard]

In D127267#3568388, @rprichard wrote:

Aside: There are only two calls to AllocateTarget: one in TargetInfo::CreateTargetInfo and one in NVPTXTargetInfo::NVPTXTargetInfo. This change removes NVPTXTargetInfo's call.

There are various places that call clang::TargetInfo::CreateTargetInfo, though, so if a call site had been providing a clang::TargetOptions with a set HostTriple field, but not calling clang::TargetInfo::setAuxTarget, then maybe the NVPTXTargetInfo behavior could change. I'll audit the calls of clang::TargetInfo::CreateTargetInfo.

[yaxunl]

This patch is to fix an issue, right? At least we need a test to prevent that issue from happening again.

[rprichard]

In D127267#3570269, @yaxunl wrote:

This patch is to fix an issue, right? At least we need a test to prevent that issue from happening again.

Yes, this patch is necessary to keep the clang/test/Preprocessor/cuda-types.cu test passing after applying D28213. That test is verifying that __GCC_ATOMIC_LLONG_LOCK_FREE is the same for --cuda-device-only and --cuda-host-only.

D28213 fixes the value of __GCC_ATOMIC_LLONG_LOCK_FREE to be 2 when targeting 586 and up, which has the cx8 feature (cmpxchg8b). Without this NVPTX patch, the value of __GCC_ATOMIC_LLONG_LOCK_FREE is 2 for --cuda-host-only, but 1 for --cuda-device-only, because when NVPTXTargetInfo creates the host TargetInfo, it only uses the host triple, and doesn't set the target CPU nor initialize CPU features. (Specifically, NVPTXTargetInfo calls AllocateTarget but skips all the other work that TargetInfo::CreateTargetInfo does, like calling initFeatureMap, handleTargetFeatures, and setMaxAtomicWidth.)

[tra]

I think the root cause of the problem here is that CUDA compilation assumes that the GPU-side looks identical to the host side of the compilation. The test was intended to verify that and, AFAICT, it did its job flagging the issue here.

AFAICT, __GCC_ATOMIC_LLONG_LOCK_FREE=1 is the correct setting for the GPU.

In this particular case host and GPU do have different capabilities and the compiler will see a different subset of preprocessed sources, that's unfortunate, but unavoidable, unless we force the host to have value of 1.

Setting _GCC_ATOMIC_LLONG_LOCK_FREE=2 on the GPU side would result in an invalid code and we definitely do not want to do that.

I think the right thing to do here is to add __GCC_ATOMIC_LLONG_LOCK_FREE= to the list of ignored differences in the clang/test/Preprocessor/cuda-types.cu, similar to how we handle LONG_DOUBLE-related macros there.

[rprichard]

Ok, I can upload a patch omitting *_ATOMIC_LLONG_LOCK_FREE from the macro testing.

FWIW, did you see this comment in clang/lib/Basic/Targets/NVPTX.cpp?:

// This is a bit of a lie, but it controls __GCC_ATOMIC_XXX_LOCK_FREE, and
// we need those macros to be identical on host and device, because (among
// other things) they affect which standard library classes are defined, and
// we need all classes to be defined on both the host and device.
MaxAtomicInlineWidth = HostTarget->getMaxAtomicInlineWidth();

It was added in D24407, apparently so that __GCC_ATOMIC_INT_LOCK_FREE would be 2 instead of 1. I imagine that's OK, because NVPTX does have atomic int?

[rprichard]

I uploaded D127465, weakening the test.

[rprichard]

Abandoning this patch because D127465 is a better fix.

clang/include/clang/Basic/TargetInfo.h

virtual bool validateTarget(DiagnosticsEngine &Diags) const {
     return true;
   }
 
   /// Check that OpenCL target has valid options setting based on OpenCL
   /// version.
   virtual bool validateOpenCLTarget(const LangOptions &Opts,
                                     DiagnosticsEngine &Diags) const;
 
-  virtual void setAuxTarget(const TargetInfo *Aux) {}
+  virtual void setAuxTarget(IntrusiveRefCntPtr<TargetInfo> Aux) {}
 
   /// Whether target allows debuginfo types for decl only variables/functions.
   virtual bool allowDebugInfoForExternalRef() const { return false; }
 
   /// Returns the darwin target variant triple, the variant of the deployment
   /// target for which the code is being compiled.
   const llvm::Triple *getDarwinTargetVariantTriple() const {
     return DarwinTargetVariantTriple ? DarwinTargetVariantTriple.getPointer()

clang/include/clang/Basic/TargetOptions.h

 namespace clang {
 
 /// Options for controlling the target.
 class TargetOptions {
 public:
   /// The name of the target triple to compile for.
   std::string Triple;
 
-  /// When compiling for the device side, contains the triple used to compile
-  /// for the host.
-  std::string HostTriple;
-
   /// If given, the name of the target CPU to generate code for.
   std::string CPU;
 
   /// If given, the name of the target CPU to tune code for.
   std::string TuneCPU;
 
   /// If given, the unit to use for floating point math.
   std::string FPMath;

clang/lib/Basic/Targets/AMDGPU.h

public:
   // address space has value 0 but in private and local address space has
   // value ~0.
   uint64_t getNullPointerValue(LangAS AS) const override {
     // FIXME: Also should handle region.
     return (AS == LangAS::opencl_local || AS == LangAS::opencl_private)
       ? ~0 : 0;
   }
 
-  void setAuxTarget(const TargetInfo *Aux) override;
+  void setAuxTarget(IntrusiveRefCntPtr<TargetInfo> Aux) override;
 
   bool hasBitIntType() const override { return true; }
 
   // Record offload arch features since they are needed for defining the
   // pre-defined macros.
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override {
     auto TargetIDFeatures =

clang/lib/Basic/Targets/AMDGPU.cpp

void AMDGPUTargetInfo::getTargetDefines(const LangOptions &Opts,
   if (hasFP64())
     Builder.defineMacro("__HAS_FP64__");
   if (hasFastFMA())
     Builder.defineMacro("FP_FAST_FMA");
 
   Builder.defineMacro("__AMDGCN_WAVEFRONT_SIZE", Twine(WavefrontSize));
 }
 
-void AMDGPUTargetInfo::setAuxTarget(const TargetInfo *Aux) {
+void AMDGPUTargetInfo::setAuxTarget(IntrusiveRefCntPtr<TargetInfo> Aux) {
   assert(HalfFormat == Aux->HalfFormat);
   assert(FloatFormat == Aux->FloatFormat);
   assert(DoubleFormat == Aux->DoubleFormat);
 
   // On x86_64 long double is 80-bit extended precision format, which is
   // not supported by AMDGPU. 128-bit floating point format is also not
   // supported by AMDGPU. Therefore keep its own format for these two types.
   auto SaveLongDoubleFormat = LongDoubleFormat;
   auto SaveFloat128Format = Float128Format;
-  copyAuxTarget(Aux);
+  copyAuxTarget(Aux.get());
   LongDoubleFormat = SaveLongDoubleFormat;
   Float128Format = SaveFloat128Format;
   // For certain builtin types support on the host target, claim they are
   // support to pass the compilation of the host code during the device-side
   // compilation.
   // FIXME: As the side effect, we also accept `__float128` uses in the device
   // code. To rejct these builtin types supported in the host target but not in
   // the device target, one approach would support `device_builtin` attribute
   // so that we could tell the device builtin types from the host ones. The
   // also solves the different representations of the same builtin type, such
   // as `size_t` in the MSVC environment.
   if (Aux->hasFloat128Type()) {
     HasFloat128 = true;
     Float128Format = DoubleFormat;
   }
 }

clang/lib/Basic/Targets/NVPTX.h

static const int NVPTXDWARFAddrSpaceMap[] = {
     4,  // opencl_constant or cuda_constant
 };
 
 class LLVM_LIBRARY_VISIBILITY NVPTXTargetInfo : public TargetInfo {
   static const char *const GCCRegNames[];
   static const Builtin::Info BuiltinInfo[];
   CudaArch GPU;
   uint32_t PTXVersion;
-  std::unique_ptr<TargetInfo> HostTarget;
+  IntrusiveRefCntPtr<TargetInfo> HostTarget;
 
 public:
   NVPTXTargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts,
                   unsigned TargetPointerWidth);
 
+  void setAuxTarget(IntrusiveRefCntPtr<TargetInfo> Aux) override;
+
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override;
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override;
 
   bool
   initFeatureMap(llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags,
                  StringRef CPU,

clang/lib/Basic/Targets/NVPTX.cpp

NVPTXTargetInfo::NVPTXTargetInfo(const llvm::Triple &Triple,
   if (TargetPointerWidth == 32)
     resetDataLayout("e-p:32:32-i64:64-i128:128-v16:16-v32:32-n16:32:64");
   else if (Opts.NVPTXUseShortPointers)
     resetDataLayout(
         "e-p3:32:32-p4:32:32-p5:32:32-i64:64-i128:128-v16:16-v32:32-n16:32:64");
   else
     resetDataLayout("e-i64:64-i128:128-v16:16-v32:32-n16:32:64");
 
-  // If possible, get a TargetInfo for our host triple, so we can match its
-  // types.
-  llvm::Triple HostTriple(Opts.HostTriple);
-  if (!HostTriple.isNVPTX())
-    HostTarget.reset(AllocateTarget(llvm::Triple(Opts.HostTriple), Opts));
-
-  // If no host target, make some guesses about the data layout and return.
-  if (!HostTarget) {
-    LongWidth = LongAlign = TargetPointerWidth;
-    PointerWidth = PointerAlign = TargetPointerWidth;
-    switch (TargetPointerWidth) {
-    case 32:
-      SizeType = TargetInfo::UnsignedInt;
-      PtrDiffType = TargetInfo::SignedInt;
-      IntPtrType = TargetInfo::SignedInt;
-      break;
-    case 64:
-      SizeType = TargetInfo::UnsignedLong;
-      PtrDiffType = TargetInfo::SignedLong;
-      IntPtrType = TargetInfo::SignedLong;
-      break;
-    default:
-      llvm_unreachable("TargetPointerWidth must be 32 or 64");
-    }
-    return;
-  }
+  // Set defaults, which will be overridden if there is a host/aux target.
 
+void NVPTXTargetInfo::setAuxTarget(IntrusiveRefCntPtr<TargetInfo> Aux) {
+  // Retain the host TargetInfo to use later for checking calling conventions.
+  HostTarget = Aux;
+
   // Copy properties from host target.
   PointerWidth = HostTarget->getPointerWidth(/* AddrSpace = */ 0);
   PointerAlign = HostTarget->getPointerAlign(/* AddrSpace = */ 0);
   BoolWidth = HostTarget->getBoolWidth();
   BoolAlign = HostTarget->getBoolAlign();
   IntWidth = HostTarget->getIntWidth();
   IntAlign = HostTarget->getIntAlign();
   HalfWidth = HostTarget->getHalfWidth();

clang/lib/Frontend/CompilerInstance.cpp

if (!getAuxTarget() &&
        getLangOpts().SYCLIsDevice) &&
       !getFrontendOpts().AuxTriple.empty()) {
     auto TO = std::make_shared<TargetOptions>();
     TO->Triple = llvm::Triple::normalize(getFrontendOpts().AuxTriple);
     if (getFrontendOpts().AuxTargetCPU)
       TO->CPU = getFrontendOpts().AuxTargetCPU.getValue();
     if (getFrontendOpts().AuxTargetFeatures)
       TO->FeaturesAsWritten = getFrontendOpts().AuxTargetFeatures.getValue();
-    TO->HostTriple = getTarget().getTriple().str();
     setAuxTarget(TargetInfo::CreateTargetInfo(getDiagnostics(), TO));
   }
 
   if (!getTarget().hasStrictFP() && !getLangOpts().ExpStrictFP) {
     if (getLangOpts().getFPRoundingMode() !=
         llvm::RoundingMode::NearestTiesToEven) {
       getDiagnostics().Report(diag::warn_fe_backend_unsupported_fp_rounding);
       getLangOpts().setFPRoundingMode(llvm::RoundingMode::NearestTiesToEven);
bool CompilerInstance::createTarget() {
   // Inform the target of the language options.
   // FIXME: We shouldn't need to do this, the target should be immutable once
   // created. This complexity should be lifted elsewhere.
   getTarget().adjust(getDiagnostics(), getLangOpts());
 
   // Adjust target options based on codegen options.
   getTarget().adjustTargetOptions(getCodeGenOpts(), getTargetOpts());
 
-  if (auto *Aux = getAuxTarget())
-    getTarget().setAuxTarget(Aux);
+  if (AuxTarget)
+    getTarget().setAuxTarget(AuxTarget);
 
   return true;
 }
 
 llvm::vfs::FileSystem &CompilerInstance::getVirtualFileSystem() const {
   return getFileManager().getVirtualFileSystem();
 }
 

clang/lib/Frontend/CompilerInvocation.cpp

bool CompilerInvocation::CreateFromArgsImpl(
 
   for (auto Warning : Res.getDiagnosticOpts().Warnings) {
     if (Warning == "misexpect" &&
         !Diags.isIgnored(diag::warn_profile_data_misexpect, SourceLocation())) {
       Res.getCodeGenOpts().MisExpect = true;
     }
   }
 
-  if (LangOpts.CUDA) {
-    // During CUDA device-side compilation, the aux triple is the
-    // triple used for host compilation.
-    if (LangOpts.CUDAIsDevice)
-      Res.getTargetOpts().HostTriple = Res.getFrontendOpts().AuxTriple;
-  }
-
-  // Set the triple of the host for OpenMP device compile.
-  if (LangOpts.OpenMPIsDevice)
-    Res.getTargetOpts().HostTriple = Res.getFrontendOpts().AuxTriple;
-
   ParseCodeGenArgs(Res.getCodeGenOpts(), Args, DashX, Diags, T,
                    Res.getFrontendOpts().OutputFile, LangOpts);
 
   // FIXME: Override value name discarding when asan or msan is used because the
   // backend passes depend on the name of the alloca in order to print out
   // names.
   Res.getCodeGenOpts().DiscardValueNames &=
       !LangOpts.Sanitize.has(SanitizerKind::Address) &&