[PATCH] D56318: [HIP] Fix size_t for MSVC environment

This revision was automatically updated to reflect the committed changes. Closed by commit rC352620: [HIP] Fix size_t for MSVC environment (authored by yaxunl, committed by ). Changed prior to commit: https://reviews.llvm.org/D56318?vs=184245&id=184274#toc Repository: rC Clang CHANGES SINCE

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56318/new/ https://reviews.llvm.org/D56318 ___ cfe-commits mailing list

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl updated this revision to Diff 184245. yaxunl added a comment. Use const argument. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56318/new/ https://reviews.llvm.org/D56318 Files: include/clang/Basic/TargetInfo.h lib/Basic/TargetInfo.cpp lib/Basic/Targets/AMDGPU.cpp lib/Ba

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. One minor change and then LGTM. Comment at: include/clang/Basic/TargetInfo.h:1352 + /// Copy type and layout related info. + void copyAuxTarget(TargetInfo *Aux); virtual uint64_t getPointerWidthV(unsigned AddrSpace) const { This c

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl updated this revision to Diff 184238. yaxunl added a comment. Revised by John's comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56318/new/ https://reviews.llvm.org/D56318 Files: include/clang/Basic/TargetInfo.h lib/Basic/TargetInfo.cpp lib/Basic/Targets/AMDGPU.cpp

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:52 -/// Exposes information about the current target. -/// -class TargetInfo : public RefCountedBase { - std::shared_ptr TargetOpts; - llvm::Triple Triple; -protected: - // Target values set by the

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl added a comment. 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

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl updated this revision to Diff 182815. yaxunl added a comment. separate layout controlling flags to a base class for TargetInfo. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56318/new/ https://reviews.llvm.org/D56318 Files: include/clang/Basic/TargetInfo.h lib/Basic/TargetIn

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. 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

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl updated this revision to Diff 181326. yaxunl added a comment. Herald added a subscriber: jfb. Copy type information from AuxTarget. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56318/new/ https://reviews.llvm.org/D56318 Files: include/clang/Basic/TargetInfo.h lib/Basic/Targ

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl added a comment. In D56318#1353176 , @rjmccall wrote: > In D56318#1353116 , @yaxunl wrote: > > > In D56318#1353106 , @rjmccall > > wrote: > > > > > No, I understand t

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. 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

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl added a comment. 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

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. 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`. CH

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl added a comment. 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

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. 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 select

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl added a comment. 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 t

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. 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? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D5

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl added a comment. 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.

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. 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 compi

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl added a comment. 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 pr

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. 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. CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

yaxunl created this revision. yaxunl added reviewers: tra, rjmccall. Herald added subscribers: tpr, nhaehnle, jvesely. In 64 bit MSVC environment size_t is defined as unsigned long long. Fix AMDGPU target info to match it in MSVC environment. https://reviews.llvm.org/D56318 Files: lib/Basic/