[HIP] Fix size_t for MSVC environment

[yaxunl]

Summary

In 64 bit MSVC environment size_t is defined as unsigned long long. Fix AMDGPU target info to match it in MSVC environment.

[rjmccall]

What's the general idea here, that you're going to pretend to be the environment's "standard" CPU target of the right pointer width and try to match the ABI exactly? This seems like a pretty treacherous road to go down.

[yaxunl]

In D56318#1346456, @rjmccall wrote:

What's the general idea here, that you're going to pretend to be the environment's "standard" CPU target of the right pointer width and try to match the ABI exactly? This seems like a pretty treacherous road to go down.

The pointer width does not change. In both case it is 64 bit. The only difference is that MSVC uses unsigned long long as size_t whereas by default AMDGPU uses unsigned long as size_t. They have the same size but in AST they are different type. When HIP is compiled in MSVC environment, it has to use header files of MSVC. This nominal difference in size_t definition causes compilation error since MSVC header files contains typedef of size_t as unsigned long long. Since we cannot change header files of MSVC, we have to change our own size_t definition.

We do not want to change our device ABI.

[rjmccall]

No, no, I understand that you're not changing pointer sizes, but this is one example of trying to match the ABI of the target environment, and I'm trying to understand how far that goes. What does it mean to be in the "MSVC" environment when you're actually just compiling for the GPU? Why are you using OS headers in the first place? Do you need struct layout to match MSVC (presumably on x86-64)? What should happen with the C++ ABI?

[yaxunl]

In D56318#1346693, @rjmccall wrote:

No, no, I understand that you're not changing pointer sizes, but this is one example of trying to match the ABI of the target environment, and I'm trying to understand how far that goes. What does it mean to be in the "MSVC" environment when you're actually just compiling for the GPU? Why are you using OS headers in the first place? Do you need struct layout to match MSVC (presumably on x86-64)? What should happen with the C++ ABI?

HIP is single source program. The same source code is compiled for both host and device. Since HIP is an extension to C++, it uses the C++ header files of the system. This is true for both host code and device code. On linux, both uses gcc header files. On windows, when MSVC is installed and default target environment is MSVC, the host compilation will use MSVC header files, so does the device compilation. For device compilation, most of the stuff in MSVC headers do not matter, e.g. function declarations, since they are for host. What matters are mostly type definitions. They should be consistent for both device and host. Since MSVC supports C++11, it should work. As an example, CUDA SDK supports MSVC.

In this patch, the driver checks the host target triple environment, if it is MSVC, it will attach -msvc to the device target triple which will be passed to device compilation. Then device compilation knows that it is using the MSVC header files and will make adjustment to be consistent with it.

[rjmccall]

Okay. Is there a reasonable way to make your targets delegate to a different TargetInfo implementation for most things so that you can generally match the host target for things like type sizes and alignments?

[yaxunl]

In D56318#1346991, @rjmccall wrote:

Okay. Is there a reasonable way to make your targets delegate to a different TargetInfo implementation for most things so that you can generally match the host target for things like type sizes and alignments?

There is TargetInfo for AuxTarget. In this case, the main target is amdgpu and the AuxTarget is x86_64. I am thinking maybe I can add a SizeTTarget pointer to ASTContext, and add a hook shouldDelegateSizeTTypeToAuxTarget to TargetInfo. If it is true, then ASTContext use size_t type in AuxTarget.

[rjmccall]

If I was only concerned about size_t, your current solution would be fine. My concern is that you really need to match *all* of the associated CPU target's ABI choices, so your target really ought to be forwarding everything to that target by default and only selectively overriding it in order to support GPU-specific features. Probably the easiest way to do that is via inheritance.

[yaxunl]

In D56318#1352962, @rjmccall wrote:

If I was only concerned about size_t, your current solution would be fine. My concern is that you really need to match *all* of the associated CPU target's ABI choices, so your target really ought to be forwarding everything to that target by default and only selectively overriding it in order to support GPU-specific features. Probably the easiest way to do that is via inheritance.

We only need to match the type size and alignment in device and host compilation, but do not need to match function call ABI. In fact our backend has its own function ABI which is different from host on linux, but it does not preventing us from supporting HIP on linux. This is because the device kernel is launched through HIP runtime, which gets kernel argument size and offset from kernel image, and lays out the arguments for the kernel.

The latest CUDA kernel launching API cuLaunchKernel does similar thing (https://docs.nvidia.com/cuda/cuda-driver-api/group__CUDA__EXEC.html#group__CUDA__EXEC_1gb8f3dc3031b40da29d5f9a7139e52e15) . Basically the host code only needs to pass an array of pointer to the arguments, whereas "the number of kernel parameters and their offsets and sizes do not need to be specified as that information is retrieved directly from the kernel's image".

If the device backend has to switch to different ABI according to host environment, that will be very painful for the backend.

[rjmccall]

No, I understand that things like the function-call ABI should be different from the associated host ABI, but things like the size of long and the bit-field layout algorithm presumably shouldn't be, and that's the sort of thing that's configured by TargetInfo.

[yaxunl]

In D56318#1353106, @rjmccall wrote:

No, I understand that things like the function-call ABI should be different from the associated host ABI, but things like the size of long and the bit-field layout algorithm presumably shouldn't be, and that's the sort of thing that's configured by TargetInfo.

How about create a ForwardingTargegInfo which will has a pointer to AuxTarget and forward to that target if it is not null. Then let AMDGPUTargetInfo inherit from that.

[rjmccall]

In D56318#1353116, @yaxunl wrote:

In D56318#1353106, @rjmccall wrote:

No, I understand that things like the function-call ABI should be different from the associated host ABI, but things like the size of long and the bit-field layout algorithm presumably shouldn't be, and that's the sort of thing that's configured by TargetInfo.

How about create a ForwardingTargegInfo which will has a pointer to AuxTarget and forward to that target if it is not null. Then let AMDGPUTargetInfo inherit from that.

Why forward? You have, like, two supported host environments, right? Can you just a subclass apiece of either MicrosoftX86_64TargetInfo or X86_64TargetInfo?

If that's unreasonable and you do need to forward, having a ForwardingTargetInfo sounds like a good idea, although I think you should require it to have an underlying target, and I think you need it to copy all the fields of that target.

[yaxunl]

In D56318#1353176, @rjmccall wrote:

In D56318#1353116, @yaxunl wrote:

In D56318#1353106, @rjmccall wrote:

No, I understand that things like the function-call ABI should be different from the associated host ABI, but things like the size of long and the bit-field layout algorithm presumably shouldn't be, and that's the sort of thing that's configured by TargetInfo.

How about create a ForwardingTargegInfo which will has a pointer to AuxTarget and forward to that target if it is not null. Then let AMDGPUTargetInfo inherit from that.

Why forward? You have, like, two supported host environments, right? Can you just a subclass apiece of either MicrosoftX86_64TargetInfo or X86_64TargetInfo?

If that's unreasonable and you do need to forward, having a ForwardingTargetInfo sounds like a good idea, although I think you should require it to have an underlying target, and I think you need it to copy all the fields of that target.

There are lots of child class of X86_64TargetInfo, e.g., CygwinX86_64TargetInfo, MicrosoftX86_64TargetInfo, MinGWX86_64TargetInfo, etc. to inherit each one of them will result in duplicated code. Also, many stuff in these TargetInfo do not apply to AMDGPU target. I think I should only selectively copy the relevant fields.

[yaxunl]

Copy type information from AuxTarget.

[rjmccall]

It's pretty unfortunate that all these fields have to be individually called out like this. Can you move all these basic layout fields into a separate struct (which can be a secondary base class of TargetInfo) which can then just be normally copied? Anything that needs special copy semantics, like the LLVM DataLayout (do you need to copy this?) doesn't need to go into that struct, just the basic POD things that determine fundamental type layouts and semantics.

[yaxunl]

separate layout controlling flags to a base class for TargetInfo.

[yaxunl]

In D56318#1355705, @rjmccall wrote:

It's pretty unfortunate that all these fields have to be individually called out like this. Can you move all these basic layout fields into a separate struct (which can be a secondary base class of TargetInfo) which can then just be normally copied? Anything that needs special copy semantics, like the LLVM DataLayout (do you need to copy this?) doesn't need to go into that struct, just the basic POD things that determine fundamental type layouts and semantics.

LLVM DataLayout contains target specific stuff and cannot be simply copied. So far we did not see necessity to adjust device data layout for host.

[rjmccall]

include/clang/Basic/TargetInfo.h

50–54

"Fields controlling how types are laid out in memory; these may need to be copied for targets like AMDGPU that base their ABIs on an auxiliary CPU target."

196

Why is this flag necessary? Can't setAuxTarget just decide whether or not to copy?

Specifically, I would suggest:

• Make copyAuxTarget be a non-virtual protected method that unconditionally copies the target.
• Make setAuxTarget a virtual method that does nothing by default.
• Override setAuxTarget for AMDGPU and call copyAuxTarget.

[yaxunl]

Revised by John's comments.

[rjmccall]

One minor change and then LGTM.

include/clang/Basic/TargetInfo.h

1352

This can take a const TargetInfo *, which also very clearly documents expectations.

[yaxunl]

Use const argument.

[rjmccall]

Thanks, LGTM.

include/clang/Basic/TargetInfo.h

 class LangOptions;
 class CodeGenOptions;
 class MacroBuilder;
 class QualType;
 class SourceLocation;
 class SourceManager;
 
 namespace Builtin { struct Info; }
 
-/// Exposes information about the current target.
-///
-class TargetInfo : public RefCountedBase<TargetInfo> {
-  std::shared_ptr<TargetOptions> TargetOpts;
-  llvm::Triple Triple;
-protected:
-  // Target values set by the ctor of the actual target implementation.  Default
-  // values are specified by the TargetInfo constructor.
-  bool BigEndian;
-  bool TLSSupported;
-  bool VLASupported;
-  bool NoAsmVariants;  // True if {|} are normal characters.
-  bool HasLegalHalfType; // True if the backend supports operations on the half
-                         // LLVM IR type.
-  bool HasFloat128;
-  bool HasFloat16;
+/// Fields controlling how types are laid out in memory; these may need to
+/// be copied for targets like AMDGPU that base their ABIs on an auxiliary
+/// CPU target.
+struct TransferrableTargetInfo {
   unsigned char PointerWidth, PointerAlign;
   unsigned char BoolWidth, BoolAlign;
   unsigned char IntWidth, IntAlign;
   unsigned char HalfWidth, HalfAlign;
   unsigned char FloatWidth, FloatAlign;
   unsigned char DoubleWidth, DoubleAlign;
   unsigned char LongDoubleWidth, LongDoubleAlign, Float128Align;
   unsigned char LargeArrayMinWidth, LargeArrayAlign;
struct TransferrableTargetInfo {
   // sign if PaddingOnUnsignedFixedPoint is set.
   unsigned char ShortAccumScale;
   unsigned char AccumScale;
   unsigned char LongAccumScale;
 
   unsigned char SuitableAlign;
   unsigned char DefaultAlignForAttributeAligned;
   unsigned char MinGlobalAlign;
-  unsigned char MaxAtomicPromoteWidth, MaxAtomicInlineWidth;
+
+  unsigned short NewAlign;
   unsigned short MaxVectorAlign;
   unsigned short MaxTLSAlign;
+
+  const llvm::fltSemantics *HalfFormat, *FloatFormat, *DoubleFormat,
+    *LongDoubleFormat, *Float128Format;
+
+  ///===---- Target Data Type Query Methods -------------------------------===//
+  enum IntType {
+    NoInt = 0,
+    SignedChar,
+    UnsignedChar,
+    SignedShort,
+    UnsignedShort,
+    SignedInt,
+    UnsignedInt,
+    SignedLong,
+    UnsignedLong,
+    SignedLongLong,
+    UnsignedLongLong
+  };
+
+  enum RealType {
+    NoFloat = 255,
+    Float = 0,
+    Double,
+    LongDouble,
+    Float128
+  };
+protected:
+  IntType SizeType, IntMaxType, PtrDiffType, IntPtrType, WCharType,
+          WIntType, Char16Type, Char32Type, Int64Type, SigAtomicType,
+          ProcessIDType;
+
+  /// Whether Objective-C's built-in boolean type should be signed char.
+  ///
+  /// Otherwise, when this flag is not set, the normal built-in boolean type is
+  /// used.
+  unsigned UseSignedCharForObjCBool : 1;
+
+  /// Control whether the alignment of bit-field types is respected when laying
+  /// out structures. If true, then the alignment of the bit-field type will be
+  /// used to (a) impact the alignment of the containing structure, and (b)
+  /// ensure that the individual bit-field will not straddle an alignment
+  /// boundary.
+  unsigned UseBitFieldTypeAlignment : 1;
+
+  /// Whether zero length bitfields (e.g., int : 0;) force alignment of
+  /// the next bitfield.
+  ///
+  /// If the alignment of the zero length bitfield is greater than the member
+  /// that follows it, `bar', `bar' will be aligned as the type of the
+  /// zero-length bitfield.
+  unsigned UseZeroLengthBitfieldAlignment : 1;
+
+  ///  Whether explicit bit field alignment attributes are honored.
+  unsigned UseExplicitBitFieldAlignment : 1;
+
+  /// If non-zero, specifies a fixed alignment value for bitfields that follow
+  /// zero length bitfield, regardless of the zero length bitfield type.
+  unsigned ZeroLengthBitfieldBoundary;
+};
+
+/// Exposes information about the current target.
+///
+class TargetInfo : public virtual TransferrableTargetInfo,
+                   public RefCountedBase<TargetInfo> {
+  std::shared_ptr<TargetOptions> TargetOpts;
+  llvm::Triple Triple;
+protected:
+  // Target values set by the ctor of the actual target implementation.  Default
+  // values are specified by the TargetInfo constructor.
+  bool BigEndian;
+  bool TLSSupported;
+  bool VLASupported;
+  bool NoAsmVariants;  // True if {|} are normal characters.
+  bool HasLegalHalfType; // True if the backend supports operations on the half
+                         // LLVM IR type.
+  bool HasFloat128;
+  bool HasFloat16;
+
+  unsigned char MaxAtomicPromoteWidth, MaxAtomicInlineWidth;
   unsigned short SimdDefaultAlign;
-  unsigned short NewAlign;
   std::unique_ptr<llvm::DataLayout> DataLayout;
   const char *MCountName;
-  const llvm::fltSemantics *HalfFormat, *FloatFormat, *DoubleFormat,
-    *LongDoubleFormat, *Float128Format;
   unsigned char RegParmMax, SSERegParmMax;
   TargetCXXABI TheCXXABI;
   const LangASMap *AddrSpaceMap;
 
   mutable StringRef PlatformName;
   mutable VersionTuple PlatformMinVersion;
 
   unsigned HasAlignMac68kSupport : 1;
   unsigned RealTypeUsesObjCFPRet : 3;
   unsigned ComplexLongDoubleUsesFP2Ret : 1;
 
   unsigned HasBuiltinMSVaList : 1;
 
   unsigned IsRenderScriptTarget : 1;
 
   // TargetInfo Constructor.  Default initializes all fields.
   TargetInfo(const llvm::Triple &T);
 
   void resetDataLayout(StringRef DL) {
     DataLayout.reset(new llvm::DataLayout(DL));
   }
 
 public:
   /// Construct a target for the given options.
   ///
   /// \param Opts - The options to use to initialize the target. The target may
   /// modify the options to canonicalize the target feature information to match
   /// what the backend expects.
   static TargetInfo *
   CreateTargetInfo(DiagnosticsEngine &Diags,
                    const std::shared_ptr<TargetOptions> &Opts);
 
   virtual ~TargetInfo();
 
   /// Retrieve the target options.
   TargetOptions &getTargetOpts() const {
     assert(TargetOpts && "Missing target options");
     return *TargetOpts;
   }
 
-  ///===---- Target Data Type Query Methods -------------------------------===//
-  enum IntType {
-    NoInt = 0,
-    SignedChar,
-    UnsignedChar,
-    SignedShort,
-    UnsignedShort,
-    SignedInt,
-    UnsignedInt,
-    SignedLong,
-    UnsignedLong,
-    SignedLongLong,
-    UnsignedLongLong
-  };
-
-  enum RealType {
-    NoFloat = 255,
-    Float = 0,
-    Double,
-    LongDouble,
-    Float128
-  };
-
   /// The different kinds of __builtin_va_list types defined by
   /// the target implementation.
   enum BuiltinVaListKind {
     /// typedef char* __builtin_va_list;
     CharPtrBuiltinVaList = 0,
 
     /// typedef void* __builtin_va_list;
     VoidPtrBuiltinVaList,
enum BuiltinVaListKind {
     //     long __fpr;
     //     void *__overflow_arg_area;
     //     void *__reg_save_area;
     //   } va_list[1];
     SystemZBuiltinVaList
   };
 
 protected:
-  IntType SizeType, IntMaxType, PtrDiffType, IntPtrType, WCharType,
-          WIntType, Char16Type, Char32Type, Int64Type, SigAtomicType,
-          ProcessIDType;
-
-  /// Whether Objective-C's built-in boolean type should be signed char.
-  ///
-  /// Otherwise, when this flag is not set, the normal built-in boolean type is
-  /// used.
-  unsigned UseSignedCharForObjCBool : 1;
-
-  /// Control whether the alignment of bit-field types is respected when laying
-  /// out structures. If true, then the alignment of the bit-field type will be
-  /// used to (a) impact the alignment of the containing structure, and (b)
-  /// ensure that the individual bit-field will not straddle an alignment
-  /// boundary.
-  unsigned UseBitFieldTypeAlignment : 1;
-
-  /// Whether zero length bitfields (e.g., int : 0;) force alignment of
-  /// the next bitfield.
-  ///
-  /// If the alignment of the zero length bitfield is greater than the member
-  /// that follows it, `bar', `bar' will be aligned as the type of the
-  /// zero-length bitfield.
-  unsigned UseZeroLengthBitfieldAlignment : 1;
-
-  ///  Whether explicit bit field alignment attributes are honored.
-  unsigned UseExplicitBitFieldAlignment : 1;
-
-  /// If non-zero, specifies a fixed alignment value for bitfields that follow
-  /// zero length bitfield, regardless of the zero length bitfield type.
-  unsigned ZeroLengthBitfieldBoundary;
-
   /// Specify if mangling based on address space map should be used or
   /// not for language specific address spaces
   bool UseAddrSpaceMapMangling;
 
 public:
   IntType getSizeType() const { return SizeType; }
   IntType getSignedSizeType() const {
     switch (SizeType) {
const llvm::VersionTuple &getSDKVersion() const {
     return getTargetOpts().SDKVersion;
   }
 
   /// Check the target is valid after it is fully initialized.
   virtual bool validateTarget(DiagnosticsEngine &Diags) const {
     return true;
   }
 
+  virtual void setAuxTarget(TargetInfo *Aux) {}
+
 protected:
+  /// Copy type and layout related info.
+  void copyAuxTarget(TargetInfo *Aux);
   virtual uint64_t getPointerWidthV(unsigned AddrSpace) const {
     return PointerWidth;
   }
   virtual uint64_t getPointerAlignV(unsigned AddrSpace) const {
     return PointerAlign;
   }
   virtual enum IntType getPtrDiffTypeV(unsigned AddrSpace) const {
     return PtrDiffType;

lib/Basic/TargetInfo.cpp

assert(getUnsignedLongAccumIBits() >= getUnsignedAccumIBits() &&
          getUnsignedAccumIBits() >= getUnsignedShortAccumIBits());
 
   // Each signed accum type has at least as many integral bits as its
   // corresponding unsigned accum type.
   assert(getShortAccumIBits() >= getUnsignedShortAccumIBits());
   assert(getAccumIBits() >= getUnsignedAccumIBits());
   assert(getLongAccumIBits() >= getUnsignedLongAccumIBits());
 }
+
+void TargetInfo::copyAuxTarget(TargetInfo *Aux) {
+  auto *Target = static_cast<TransferrableTargetInfo*>(this);
+  auto *Src = static_cast<TransferrableTargetInfo*>(Aux);
+  *Target = *Src;
+}

lib/Basic/Targets/AMDGPU.h

public:
   }
 
   // In amdgcn target the null pointer in global, constant, and generic
   // address space has value 0 but in private and local address space has
   // value ~0.
   uint64_t getNullPointerValue(LangAS AS) const override {
     return AS == LangAS::opencl_local ? ~0 : 0;
   }
+
+  void setAuxTarget(TargetInfo *Aux) override;
 };
 
 } // namespace targets
 } // namespace clang
 
 #endif // LLVM_CLANG_LIB_BASIC_TARGETS_AMDGPU_H

lib/Basic/Targets/AMDGPU.cpp

if (hasFastFMAF())
     Builder.defineMacro("FP_FAST_FMAF");
   if (hasLDEXPF())
     Builder.defineMacro("__HAS_LDEXPF__");
   if (hasFP64())
     Builder.defineMacro("__HAS_FP64__");
   if (hasFastFMA())
     Builder.defineMacro("FP_FAST_FMA");
 }
+
+void AMDGPUTargetInfo::setAuxTarget(TargetInfo *Aux) { copyAuxTarget(Aux); }

lib/Frontend/CompilerInstance.cpp

bool CompilerInstance::ExecuteAction(FrontendAction &Act) {
   //
   // FIXME: We shouldn't need to do this, the target should be immutable once
   // created. This complexity should be lifted elsewhere.
   getTarget().adjust(getLangOpts());
 
   // Adjust target options based on codegen options.
   getTarget().adjustTargetOptions(getCodeGenOpts(), getTargetOpts());
 
+  if (auto *Aux = getAuxTarget())
+    getTarget().setAuxTarget(Aux);
+
   // rewriter project will change target built-in bool type from its default.
   if (getFrontendOpts().ProgramAction == frontend::RewriteObjC)
     getTarget().noSignedCharForObjCBool();
 
   // Validate/process some options.
   if (getHeaderSearchOpts().Verbose)
     OS << "clang -cc1 version " CLANG_VERSION_STRING
        << " based upon " << BACKEND_PACKAGE_STRING

test/SemaCUDA/amdgpu-size_t.cu

+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -aux-triple x86_64-pc-windows-msvc -fms-compatibility -fcuda-is-device -fsyntax-only -verify %s
+
+// expected-no-diagnostics
+typedef unsigned __int64 size_t;
+typedef __int64 intptr_t;
+typedef unsigned __int64 uintptr_t;
+