[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

dexonsmith added a comment. In D77621#2002430 , @dblaikie wrote: > (seems a bit questionable to rely on uintptr_t being necessarily the same > type as either uint32_t or uint64_t - but maybe that's guaranteed/written > down somewhere)? I think in

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

browneee added a comment. https://reviews.llvm.org/D79214 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/ https://reviews.llvm.org/D77621 ___ cfe-commits mailing list

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

browneee added a comment. In D77621#2013546 , @nikic wrote: > @browneee Looks like LLVM already defines `LLVM_PTR_SIZE` as a more portable > version of `__SIZEOF_POINTER__`. I saw LLVM_PTR_SIZE, but its definition may be based on sizeof()

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

nikic added a comment. @browneee Looks like LLVM already defines `LLVM_PTR_SIZE` as a more portable version of `__SIZEOF_POINTER__`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/ https://reviews.llvm.org/D77621

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

dblaikie added a comment. In D77621#2013400 , @browneee wrote: > Thanks for the tips, MaskRay. > > Yes, I expect this would fix that issue. > > smeenai, SizeTypeMax() is intended to return size_t. > > --- > > I see a couple of options for fixing the

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

browneee added a comment. Thanks for the tips, MaskRay. Yes, I expect this would fix that issue. smeenai, SizeTypeMax() is intended to return size_t. --- I see a couple of options for fixing the truncation warning on 32-bit platforms: 1. Add an explicit cast to remove the warning. -

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

dblaikie added inline comments. Comment at: llvm/include/llvm/ADT/SmallVector.h:52 + // The maximum size depends on size_type used. + static constexpr size_t SizeMax() { +return std::numeric_limits::max(); smeenai wrote: > browneee wrote: > > dexonsmith

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

smeenai added inline comments. Comment at: llvm/include/llvm/ADT/SmallVector.h:52 + // The maximum size depends on size_type used. + static constexpr size_t SizeMax() { +return std::numeric_limits::max(); browneee wrote: > dexonsmith wrote: > > STL data

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

MaskRay added a comment. I guess this fixes https://bugs.llvm.org/show_bug.cgi?id=45289 as well Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/ https://reviews.llvm.org/D77621 ___ cfe-commits

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

MaskRay closed this revision. MaskRay added a comment. @browneee Hi, it seems that you did not attach `Differential Revision: ` to the commit dda3c19a3618dce9492687f8e880e7a73486ee98 so the differential was not closed

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

dblaikie accepted this revision. dblaikie added a comment. Seems good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/ https://reviews.llvm.org/D77621 ___ cfe-commits mailing list

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

browneee marked an inline comment as done. browneee added inline comments. Comment at: llvm/include/llvm/ADT/SmallVector.h:19 #include "llvm/Support/Compiler.h" +#include "llvm/Support/ErrorHandling.h" #include "llvm/Support/MathExtras.h" nikic wrote: > Is

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

browneee updated this revision to Diff 260064. browneee added a comment. Change uintptr_t to uint64_t to ensure this does not instantiate the same template twice on platforms where uintptr_t is equivalent to uint32_t. Also considered using the preprocessor to disable the uintptr_t

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

browneee added a comment. Reverted in 5cb4c3776a34d48e43d9118921d2191aee0e3d21 Fails on plaforms where uintptr_t is the same type as uint32_t. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

browneee closed this revision. browneee added a comment. Comitted: b5f0eae1dc3c09c020cdf9d07238dec9acdacf5f Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

browneee updated this revision to Diff 260006. browneee marked 4 inline comments as done. browneee added a comment. - Change SizeTypeMax to a static constexpr function. - Fix comment typos. - Add comment to alert others to possible performance loss if that function is moved to the header.

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

nikic added inline comments. Comment at: llvm/include/llvm/ADT/SmallVector.h:84 +template const size_t SmallVectorBase::SizeTypeMax; + dblaikie wrote: > nikic wrote: > > Is this needed? I don't think it makes a lot of sense to allow odr-use of > >

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

dblaikie added inline comments. Comment at: llvm/include/llvm/ADT/SmallVector.h:84 +template const size_t SmallVectorBase::SizeTypeMax; + nikic wrote: > Is this needed? I don't think it makes a lot of sense to allow odr-use of > `SizeTypeMax`. As it's a

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

nikic accepted this revision. nikic added inline comments. Comment at: llvm/include/llvm/ADT/SmallVector.h:19 #include "llvm/Support/Compiler.h" +#include "llvm/Support/ErrorHandling.h" #include "llvm/Support/MathExtras.h" Is this include still needed?

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

dblaikie accepted this revision. dblaikie added a comment. In D77621#2001099 , @dexonsmith wrote: > In D77621#2000237 , @nikic wrote: > > > Okay, I'm basically fine with that, if it is our stance that SmallVector

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

browneee updated this revision to Diff 259949. browneee added a comment. Switch back to size and capacity type conditionally larger approach (appologies for the noise here). Apply performance regression solutions from @nikic Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

browneee updated this revision to Diff 259480. browneee added a comment. Switch approach back to std::vector change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/ https://reviews.llvm.org/D77621 Files:

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

browneee added a comment. Thanks for the revert explanation and notes, nikic. @dexonsmith what is your current thinking on going back to the original std::vector approach? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

nikic reopened this revision. nikic added a comment. This revision is now accepted and ready to land. I have reverted this change, because it causes a 1% compile-time

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

dexonsmith closed this revision. dexonsmith added a comment. In D77621#1979769 , @browneee wrote: > GIT_COMMITTER_NAME=Andrew Browne > GIT_COMMITTER_EMAIL=brown...@google.com > > This would be my second commit. I will request access next time - thanks >

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

browneee updated this revision to Diff 258444. browneee added a comment. Rebase to latest HEAD. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/ https://reviews.llvm.org/D77621 Files: llvm/include/llvm/ADT/SmallVector.h

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

browneee updated this revision to Diff 257861. browneee added a comment. Rebase to latest HEAD. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/ https://reviews.llvm.org/D77621 Files: llvm/include/llvm/ADT/SmallVector.h

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

browneee added a comment. GIT_COMMITTER_NAME=Andrew Browne GIT_COMMITTER_EMAIL=brown...@google.com This would be my second commit. I will request access next time - thanks @dexonsmith! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

dexonsmith added a comment. In D77621#1979183 , @browneee wrote: > @dexonsmith I am not a committer, if the last changes looks good please > submit for me. Thanks! You've had a few patches in the past, I suggest you get yourself access.

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

browneee added a comment. I'm open to suggestions to resolve the clang tidy naming warnings. I would prefer to leave grow_pod the same, to minimize changes. @dexonsmith I am not a committer, if the last changes looks good please submit for me. Thanks! Comment at:

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

browneee updated this revision to Diff 257130. browneee marked 3 inline comments as done. browneee added a comment. Rename SizeMax() to SizeTypeMax(). Fix max_size(). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Thanks for your patience, I missed the updates on Friday. I have a couple of optional comments inline that I don't feel strongly about. LGTM either way. In D77621#1972764

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

dblaikie added a comment. Looks good to me at this point (I have some vague quandries about whether the report_fatal_error stuff could be improved/made more clear, but couldn't come up with an actionable suggestion so far) - @dexonsmith could you check this over and offer final approval?

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

browneee updated this revision to Diff 257044. browneee added a comment. Changed SizeMax to static constexpr function. Changed static asserts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/ https://reviews.llvm.org/D77621 Files:

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

dblaikie added inline comments. Comment at: llvm/include/llvm/ADT/SmallVector.h:53 + // The maximum size depends on size_type used. + size_t SizeMax() { return size_type(-1ULL); } dblaikie wrote: > I'd probably use numeric_limits here & make this static

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

browneee updated this revision to Diff 256803. browneee marked 3 inline comments as done. browneee added a comment. Address comments from dblaikie. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/ https://reviews.llvm.org/D77621 Files:

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

dblaikie added a comment. Please update the patch description/subject line. @dexonsmith I'll leave this to you for final approval, since it was your idea/you've been touching things here. But looks like about the right direction. Comment at: