[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

@nikic, great news!  Thanks for doing the detailed analysis.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D77621#2000237 , @nikic wrote:

> > Perhaps you could move the value computation into a constexpr variable & 
> > just return that as needed. (could be a static local constexpr, I guess - 
> > to avoid the issues around linkage of constexpr member variables)
>
> The use of a function rather than a static constexpr for SizeTypeMax() was my 
> first thought as well. It seems pretty weird to me, but maybe it's enough to 
> fall one the wrong side of some inlining heuristic.
>
> The only other thing that comes to mind is that grow_pod() moved into the 
> header, which might have negative effects. It should be possible to avoid 
> that by providing explicit template instantiations for uint32_t and uintptr_t 
> in the cpp file.
>
> I'll try to figure out what the cause is, but might take me a few days.


I've tried those two things: results 
.
 From the bottom, the first commit is a rebased version of the original change, 
the second one makes `SizeTypeMax` a constant instead of a function and the 
last one moves `grow_pod` back into the C++ file (I forgot to replace the 
UINT32_MAX references in grow_pod, but I don't think it has an impact on the 
conclusion). The first change is a `+0.75%` regression, the second is neutral 
and the last one is a `-0.70%` improvement, the remaining difference is likely 
noise. So it looks like the move of `grow_pod` into the header was the culprit.

What is rather peculiar is that the picture is similar for the max-rss numbers 
.
 I believe this is because max-rss is also influenced by the size of the clang 
binary itself, and apparently the move of grow_pod into the header increased it 
a lot. (I should probably collect clang binary size to make this easy to 
verify.) That means that there doesn't seem to be much of an increase in terms 
of actually allocated heap memory due to this change.

Taking the max-rss numbers 

 across all three commits, the only part where memory usage increases 
non-trivially is the LTO `-g` link step, by about ~1%. Possibly some debuginfo 
related stuff uses `SmallVector`.

So tl;dr looks like as long as we keep `grow_pod` outside the header file, this 
change seems to be approximately free in terms of compile-time and memory usage 
both.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D77621#2000237 , @nikic wrote:

> Okay, I'm basically fine with that, if it is our stance that SmallVector 
> should always be preferred over std::vector. Not really related to this 
> revision, but it would probably help to do some renaming/aliasing to 
> facilitate that view. Right now, the number of `SmallVector` uses in 
> LLVM is really small compared to the `std::vector` uses (100 vs 6000 based 
> on a not very accurate grep). I think part of that is in the name, and 
> calling it `using Vector = SmallVector` and `using VectorImpl = 
> SmallVectorImpl` would make it a lot more obvious that this is our 
> preferred general purpose vector type, even if the stored data is not small.


Those aliases SGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D77621#157 , @dblaikie wrote:

> @nikic any sense of the noise floor/level on these measurements? It doesn't 
> /look/ like there's much left in this that would cause problems. & I assume 
> these measurements were made on an optimized build (so we don't have to try 
> to improve the unoptimized code?


The measurements are on an optimized build (default LLVM release build, so no 
LTO). The noise level on the "instructions" metric is very low, so that changes 
above 0.1% tend to be significant. The compile-time regression on the original 
change definitely wasn't noise (but the change from D77601 
 is in the noise).

>> Other pieces I see as possibly impacting compile time are:
>> 
>> 1. This correction to SmallVectorTemplateCommon::max_size().   But 
>> SizeTypeMax() is static constexpr, this seems like it could still be 
>> optimized to a constant.
>> 
>>   ```
>> 2. size_type max_size() const { return size_type(-1) / sizeof(T); } +  
>> size_type max_size() const { +return std::min(this->SizeTypeMax(), 
>> size_type(-1) / sizeof(T)); +  } ```
> 
> Perhaps you could move the value computation into a constexpr variable & just 
> return that as needed. (could be a static local constexpr, I guess - to avoid 
> the issues around linkage of constexpr member variables)

The use of a function rather than a static constexpr for SizeTypeMax() was my 
first thought as well. It seems pretty weird to me, but maybe it's enough to 
fall one the wrong side of some inlining heuristic.

The only other thing that comes to mind is that grow_pod() moved into the 
header, which might have negative effects. It should be possible to avoid that 
by providing explicit template instantiations for uint32_t and uintptr_t in the 
cpp file.

I'll try to figure out what the cause is, but might take me a few days.

>> I don't think the automatic switch of the size/capacity field has been 
>> justified well. We have plenty of SmallVectors in LLVM that are, indeed, 
>> small. There is no way an MCRelaxableFragment will ever end up storing a 
>> single instruction that is 4G large.
> 
> @nikic - can you explain the relevance of this ^ (as @dexonsmith pointed out, 
> MCRelaxableFragment doesn't look like it would be affected by this change - 
> is there something we're missing about that?)

MCRelaxableFragment also contains a SmallVector. I used this as an 
example where we use a SmallVector with a very low upper bound on the 
size. (This example is not great, because the structure is already large for 
other reasons.)

>> Similarly, I'm not really convinced about handling this in SmallVector at 
>> all. The original change here just used an std::vector in the one place 
>> where this has become an issue. That seems like a pretty good solution until 
>> there is evidence that this is really a more widespread problem.
> 
> I'm inclined to go with @dexonsmith's perspective here, as the author of the 
> original change & the general attitude that SmallVector should support this 
> kind of use case.

Okay, I'm basically fine with that, if it is our stance that SmallVector should 
always be preferred over std::vector. Not really related to this revision, but 
it would probably help to do some renaming/aliasing to facilitate that view. 
Right now, the number of `SmallVector` uses in LLVM is really small 
compared to the `std::vector` uses (100 vs 6000 based on a not very accurate 
grep). I think part of that is in the name, and calling it `using Vector = 
SmallVector` and `using VectorImpl = SmallVectorImpl` would make it 
a lot more obvious that this is our preferred general purpose vector type, even 
if the stored data is not small.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D77621#1999757 , @browneee wrote:

> I resubmitted the report_fatal_error checks again under D77601 
> 
>
> http://llvm-compile-time-tracker.com/compare.php?from=7375212172951d2fc283c81d03c1a8588c3280c6=a30e7ea88e75568feed020aedae73c52de35=max-rss
>  
> http://llvm-compile-time-tracker.com/compare.php?from=7375212172951d2fc283c81d03c1a8588c3280c6=a30e7ea88e75568feed020aedae73c52de35=instructions
>
> Imo impact from this part is insignificant.


Ah, OK - thanks for noting that!

@nikic any sense of the noise floor/level on these measurements? It doesn't 
/look/ like there's much left in this that would cause problems. & I assume 
these measurements were made on an optimized build (so we don't have to try to 
improve the unoptimized code?

> Other pieces I see as possibly impacting compile time are:
> 
> 1. This correction to SmallVectorTemplateCommon::max_size().   But 
> SizeTypeMax() is static constexpr, this seems like it could still be 
> optimized to a constant.
> 
>   ```
> 2. size_type max_size() const { return size_type(-1) / sizeof(T); } +  
> size_type max_size() const { +return std::min(this->SizeTypeMax(), 
> size_type(-1) / sizeof(T)); +  } ```

Perhaps you could move the value computation into a constexpr variable & just 
return that as needed. (could be a static local constexpr, I guess - to avoid 
the issues around linkage of constexpr member variables)

> 2. More function calls. They also appear fairly optimizable to me.
> 
>   I may not have good insight into the actual optimization behavior here.

*nod* Didn't seem especially interesting.

> I don't think the automatic switch of the size/capacity field has been 
> justified well. We have plenty of SmallVectors in LLVM that are, indeed, 
> small. There is no way an MCRelaxableFragment will ever end up storing a 
> single instruction that is 4G large.

@nikic - can you explain the relevance of this ^ (as @dexonsmith pointed out, 
MCRelaxableFragment doesn't look like it would be affected by this change - is 
there something we're missing about that?)

> Similarly, I'm not really convinced about handling this in SmallVector at 
> all. The original change here just used an std::vector in the one place where 
> this has become an issue. That seems like a pretty good solution until there 
> is evidence that this is really a more widespread problem.

I'm inclined to go with @dexonsmith's perspective here, as the author of the 
original change & the general attitude that SmallVector should support this 
kind of use case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Andrew via Phabricator via cfe-commits]

browneee added a comment.

I resubmitted the report_fatal_error checks again under D77601 


http://llvm-compile-time-tracker.com/compare.php?from=7375212172951d2fc283c81d03c1a8588c3280c6=a30e7ea88e75568feed020aedae73c52de35=max-rss
http://llvm-compile-time-tracker.com/compare.php?from=7375212172951d2fc283c81d03c1a8588c3280c6=a30e7ea88e75568feed020aedae73c52de35=instructions

Imo impact from this part is insignificant.

Other pieces I see as possibly impacting compile time are:

1. This correction to SmallVectorTemplateCommon::max_size().   But 
SizeTypeMax() is static constexpr, this seems like it could still be optimized 
to a constant.

  -  size_type max_size() const { return size_type(-1) / sizeof(T); }
  +  size_type max_size() const {
  +return std::min(this->SizeTypeMax(), size_type(-1) / sizeof(T));
  +  }



2. More function calls. They also appear fairly optimizable to me.

I may not have good insight into the actual optimization behavior here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

>   because it causes a 1% compile-time and memory usage regression.

Yeah, some memory regression is expected and, in my opinion, acceptable for the 
change.

The compile time regression presumably came from the changes to the 
report_fatal_error handling in SmallVector - perhaps it could be 
changed/omitted in this commit, and done separately to assess the cost of 
changes to that error checking?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D77621#1995673 , @browneee wrote:

> Thanks for the revert explanation and notes, nikic.
>
> @dexonsmith what is your current thinking on going back to the original 
> std::vector approach?


SmallVector has only been limited to UINT32_MAX size for about a year and I 
think it’s a pretty major regression that I broke using it for arbitrary char 
buffers.  I don’t think that’s acceptable really.  Note that there was pushback 
when I shrank SmallVector at all for aesthetic reasons.

Note that breaking `SmallVector` also breaks `SmallString` and 
`raw_svector_ostream` for buffers that are sometimes large.  This was certainly 
not the goal of my original commit and I think it’s the wrong result.

One thing to try I suppose is specializing just when `sizeof(T)==1`.  But even 
if there’s still a compile time hit, I think making SmallVector functional is 
more critical.  Use cases that really want something tiny can use 
`TinyPtrVector`; or if that’s not appropriate we can introduce a `TinyVector` 
that works for other types (could make it 8 bytes with small storage for 1 
element if the type is 4 bytes or smaller).

This might be worth a thread on llvm-dev.  Maybe no one else thinks LLVM should 
use `SmallVectorImpl` pervasively in APIs anymore.

(AFAICT `MCRelaxableFragment` has a `SmallVector` and would not have 
been affected by the reverted commit since `sizeof(MCFixup)` is quite large, 
not sure why that was brought up.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Andrew via Phabricator via cfe-commits]

browneee updated this revision to Diff 256621.
browneee added a comment.

Change to suggested approach: size and capacity type conditionally larger for 
small element types.

Also incorporate https://reviews.llvm.org/D77601


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
  llvm/lib/Support/SmallVector.cpp

Index: llvm/lib/Support/SmallVector.cpp
===
--- llvm/lib/Support/SmallVector.cpp
+++ llvm/lib/Support/SmallVector.cpp
@@ -37,29 +37,11 @@
   sizeof(unsigned) * 2 + sizeof(void *) * 2,
   "wasted space in SmallVector size 1");
 
-/// grow_pod - This is an implementation of the grow() method which only works
-/// on POD-like datatypes and is out of line to reduce code duplication.
-void SmallVectorBase::grow_pod(void *FirstEl, size_t MinCapacity,
-   size_t TSize) {
-  // Ensure we can fit the new capacity in 32 bits.
-  if (MinCapacity > UINT32_MAX)
-report_bad_alloc_error("SmallVector capacity overflow during allocation");
-
-  size_t NewCapacity = 2 * capacity() + 1; // Always grow.
-  NewCapacity =
-  std::min(std::max(NewCapacity, MinCapacity), size_t(UINT32_MAX));
-
-  void *NewElts;
-  if (BeginX == FirstEl) {
-NewElts = safe_malloc(NewCapacity * TSize);
-
-// Copy the elements over.  No need to run dtors on PODs.
-memcpy(NewElts, this->BeginX, size() * TSize);
-  } else {
-// If this wasn't grown from the inline copy, grow the allocated space.
-NewElts = safe_realloc(this->BeginX, NewCapacity * TSize);
-  }
-
-  this->BeginX = NewElts;
-  this->Capacity = NewCapacity;
-}
+// Check that SmallVector with 1 byte elements takes extra space due to using
+// uintptr_r instead of uint32_t for size and capacity.
+static_assert(sizeof(SmallVector) > sizeof(SmallVector) ||
+  sizeof(uintptr_t) == sizeof(uint32_t),
+  "1 byte elements should cause larger size and capacity type");
+static_assert(sizeof(SmallVector) ==
+  sizeof(SmallVector),
+  "larger elements should keep the same size and capacity type");
Index: llvm/include/llvm/ADT/SmallVector.h
===
--- llvm/include/llvm/ADT/SmallVector.h
+++ llvm/include/llvm/ADT/SmallVector.h
@@ -34,11 +34,23 @@
 
 namespace llvm {
 
-/// This is all the non-templated stuff common to all SmallVectors.
-class SmallVectorBase {
+/// This is all the stuff common to all SmallVectors.
+///
+/// The template parameter specifies the type which should be used to hold the
+/// Size and Capacity of the SmallVector, so it can be adjusted.
+/// Using 32 bit size is desirable to shink the size of the SmallVector.
+/// Using 64 bit size is desirable for cases like SmallVector, where a
+/// 32 bit size would limit the vector to ~4GB. SmallVectors are used for
+/// buffering bitcode output - which can exceed 4GB.
+template  class SmallVectorBase {
 protected:
+  typedef Size_T size_type;
+
   void *BeginX;
-  unsigned Size = 0, Capacity;
+  size_type Size = 0, Capacity;
+
+  // The maximum size depends on size_type used.
+  size_t SizeMax() { return size_type(-1ULL); }
 
   SmallVectorBase() = delete;
   SmallVectorBase(void *FirstEl, size_t TotalCapacity)
@@ -46,7 +58,39 @@
 
   /// This is an implementation of the grow() method which only works
   /// on POD-like data types and is out of line to reduce code duplication.
-  void grow_pod(void *FirstEl, size_t MinCapacity, size_t TSize);
+  /// This function will report a fatal error if it cannot increase capacity.
+  void grow_pod(void *FirstEl, size_t MinCapacity, size_t TSize) {
+// Ensure we can fit the new capacity.
+// This is only going to be applicable if the when the capacity is 32 bit.
+if (MinCapacity > SizeMax())
+  report_bad_alloc_error("SmallVector capacity overflow during allocation");
+
+// Ensure we can meet the guarantee of space for at least one more element.
+// The above check alone will not catch the case where grow is called with a
+// default MinCapacity of 0, but the current capacity cannot be increased.
+// This is only going to be applicable if the when the capacity is 32 bit.
+if (capacity() == SizeMax())
+  report_bad_alloc_error("SmallVector capacity unable to grow");
+
+// In theory 2*capacity can overflow if the capacity is 64 bit, but the
+// original capacity would never be large enough for this to be a problem.
+size_t NewCapacity = 2 * capacity() + 1; // Always grow.
+NewCapacity = std::min(std::max(NewCapacity, MinCapacity), SizeMax());
+
+void *NewElts;
+if (BeginX == FirstEl) {
+  NewElts = safe_malloc(NewCapacity * TSize);
+
+  // Copy the elements over.  No need to run dtors on PODs.
+  memcpy(NewElts, this->BeginX, size() 

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D77621#1970015 , @dexonsmith wrote:

> In D77621#1968647 , @browneee wrote:
>
> > Do we want to increase the complexity of SmallVector somewhat? or do we 
> > want to keep the limit and affirm SmallVector is for small things?
>
>
> I don't think we should limit `SmallVector` to small things.  Most 
> `std::string` implementations also have the small storage optimization, but 
> they're not limited to small things.  Note that even `SmallVector` has a 
> number of conveniences for LLVM over `std::vector` (such as extra API, 
> ability to use `SmallVectorImpl` APIs, and no pessimizations from exception 
> handling).
>
> Personally, I'm fine with splitting SmallVectorBase into 
> `SmallVectorBase` and `SmallVectorBase` (on 32-bit 
> architectures, there's actually no split there).  There aren't APIs that take 
> a `SmallVectorBase` so there's no downside there.  It doesn't seem too bad to 
> me to do something like:
>
>   template  SmallVectorBase {
> typedef SizeT size_type;
> // ...
>   };
>   template 
>   using SmallVectorSizeType =
>   std::conditional;
>   template  SmallVectorImpl :
>   SmallVectorBase> { ... };
>
>
> If the complexity is too much, I would personally prefer to have my patch 
> reverted (option 2 above) over making SmallVector stop working with large 
> byte arrays.


Fair enough - that complexity seems reasonably acceptable to me if you reckon 
the memory size benefits are still worthwhile (did you measure them on any 
particular workloads? Do we have lots of fairly empty SmallVectors, etc?) if 
they don't apply to smaller types like this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D77621#1968647 , @browneee wrote:

> Do we want to increase the complexity of SmallVector somewhat? or do we want 
> to keep the limit and affirm SmallVector is for small things?


I don't think we should limit `SmallVector` to small things.  Most 
`std::string` implementations also have the small storage optimization, but 
they're not limited to small things.  Note that even `SmallVector` has a 
number of conveniences for LLVM over `std::vector` (such as extra API, ability 
to use `SmallVectorImpl` APIs, and no pessimizations from exception handling).

Personally, I'm fine with splitting SmallVectorBase into 
`SmallVectorBase` and `SmallVectorBase` (on 32-bit 
architectures, there's actually no split there).  There aren't APIs that take a 
`SmallVectorBase` so there's no downside there.  It doesn't seem too bad to me 
to do something like:

  template  SmallVectorBase {
typedef SizeT size_type;
// ...
  };
  template 
  using SmallVectorSizeType =
  std::conditional;
  template  SmallVectorImpl :
  SmallVectorBase> { ... };

If the complexity is too much, I would personally prefer to have my patch 
reverted (option 2 above) over making SmallVector stop working with large byte 
arrays.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Andrew via Phabricator via cfe-commits]

browneee added a comment.

In D77621#1968437 , @dexonsmith wrote:

> This is thanks to a commit of mine that shaved a word off of `SmallVector`.  
> Some options to consider:
>
> 1. Revert to word-size integers (`size_t`? `uintptr_t`?) for Size and 
> Capacity for small-enough types.  Could be just if `sizeof(T)==1`.  Or maybe 
> just for `char` and `unsigned char`.
> 2. Revert my patch entirely and go back to words (these used to be `void*`).
> 3. (Your patch, stop using `SmallVector`.)
>
>   I think I would prefer some variation of (1) over (3).


Hi Duncan, thanks for raising these alternatives.

Links to your prior commit for context: Review 
, Commit 


I agree any of these options would solve the issue I'm experiencing.

Option 1:

- I think SmallVectorBase would need to become templated.
- The size related code would need support to two sets of edge cases.
- The varying capacity may be surprising, and adds another variation to both 
both small mode and heap mode.

Option 3:

- This patch is somewhat widespread. A more localized fix may be desirable.
- Some inconvenience of an API change for downstream.

Do we want to increase the complexity of SmallVector somewhat? or do we want to 
keep the limit and affirm SmallVector is for small things?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Andrew via Phabricator via cfe-commits]

browneee added inline comments.



Comment at: llvm/include/llvm/Bitstream/BitstreamWriter.h:19
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"

RKSimon wrote:
> Can this be dropped?
It is still used to construct records (line 512).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Andrew via Phabricator via cfe-commits]

browneee updated this revision to Diff 255891.
browneee added a comment.

Fix formatting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621

Files:
  clang-tools-extra/clang-doc/Serialize.cpp
  clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp
  clang/include/clang/Serialization/ASTWriter.h
  clang/include/clang/Serialization/PCHContainerOperations.h
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/lib/Serialization/PCHContainerOperations.cpp
  llvm/include/llvm/Bitcode/BitcodeWriter.h
  llvm/include/llvm/Bitstream/BitstreamWriter.h
  llvm/include/llvm/Remarks/BitstreamRemarkSerializer.h
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/ExecutionEngine/Orc/ThreadSafeModule.cpp
  llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
  llvm/tools/llvm-cat/llvm-cat.cpp
  llvm/tools/llvm-modextract/llvm-modextract.cpp
  llvm/unittests/Bitstream/BitstreamReaderTest.cpp
  llvm/unittests/Bitstream/BitstreamWriterTest.cpp

Index: llvm/unittests/Bitstream/BitstreamWriterTest.cpp
===
--- llvm/unittests/Bitstream/BitstreamWriterTest.cpp
+++ llvm/unittests/Bitstream/BitstreamWriterTest.cpp
@@ -8,27 +8,31 @@
 
 #include "llvm/Bitstream/BitstreamWriter.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallString.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+#include 
+
 using namespace llvm;
 
+using ::testing::IsEmpty;
+
 namespace {
 
 TEST(BitstreamWriterTest, emitBlob) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("str", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef("str\0", 4), Buffer);
+  EXPECT_EQ(StringRef("str\0", 4), StringRef(Buffer.data(), Buffer.size()));
 }
 
 TEST(BitstreamWriterTest, emitBlobWithSize) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   {
 BitstreamWriter W(Buffer);
 W.emitBlob("str");
   }
-  SmallString<64> Expected;
+  std::vector Expected;
   {
 BitstreamWriter W(Expected);
 W.EmitVBR(3, 6);
@@ -38,21 +42,21 @@
 W.Emit('r', 8);
 W.Emit(0, 8);
   }
-  EXPECT_EQ(StringRef(Expected), Buffer);
+  EXPECT_EQ(Expected, Buffer);
 }
 
 TEST(BitstreamWriterTest, emitBlobEmpty) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef(""), Buffer);
+  EXPECT_THAT(Buffer, IsEmpty());
 }
 
 TEST(BitstreamWriterTest, emitBlob4ByteAligned) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("str0", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef("str0"), Buffer);
+  EXPECT_EQ(StringRef("str0"), StringRef(Buffer.data(), Buffer.size()));
 }
 
 } // end namespace
Index: llvm/unittests/Bitstream/BitstreamReaderTest.cpp
===
--- llvm/unittests/Bitstream/BitstreamReaderTest.cpp
+++ llvm/unittests/Bitstream/BitstreamReaderTest.cpp
@@ -11,6 +11,8 @@
 #include "llvm/Bitstream/BitstreamWriter.h"
 #include "gtest/gtest.h"
 
+#include 
+
 using namespace llvm;
 
 namespace {
@@ -96,7 +98,7 @@
 StringRef BlobIn((const char *)BlobData.begin(), BlobSize);
 
 // Write the bitcode.
-SmallVector Buffer;
+std::vector Buffer;
 unsigned AbbrevID;
 {
   BitstreamWriter Stream(Buffer);
@@ -115,7 +117,7 @@
 
 // Stream the buffer into the reader.
 BitstreamCursor Stream(
-ArrayRef((const uint8_t *)Buffer.begin(), Buffer.size()));
+ArrayRef((const uint8_t *)Buffer.data(), Buffer.size()));
 
 // Header.  Included in test so that we can run llvm-bcanalyzer to debug
 // when there are problems.
Index: llvm/tools/llvm-modextract/llvm-modextract.cpp
===
--- llvm/tools/llvm-modextract/llvm-modextract.cpp
+++ llvm/tools/llvm-modextract/llvm-modextract.cpp
@@ -58,12 +58,12 @@
   ExitOnErr(errorCodeToError(EC));
 
   if (BinaryExtract) {
-SmallVector Result;
+std::vector Result;
 BitcodeWriter Writer(Result);
-Result.append(Ms[ModuleIndex].getBuffer().begin(),
+Result.insert(Result.end(), Ms[ModuleIndex].getBuffer().begin(),
   Ms[ModuleIndex].getBuffer().end());
 Writer.copyStrtab(Ms[ModuleIndex].getStrtab());
-Out->os() << Result;
+Out->os().write(Result.data(), Result.size());
 Out->keep();
 return 0;
   }
Index: llvm/tools/llvm-cat/llvm-cat.cpp
===
--- llvm/tools/llvm-cat/llvm-cat.cpp
+++ llvm/tools/llvm-cat/llvm-cat.cpp
@@ -54,7 +54,7 @@
   ExitOnError ExitOnErr("llvm-cat: ");
   LLVMContext Context;
 

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Andrew via Phabricator via cfe-commits]

browneee updated this revision to Diff 255875.
browneee marked 2 inline comments as done and an inline comment as not done.
browneee added a comment.

Build fixes in additional projects.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621

Files:
  clang-tools-extra/clang-doc/Serialize.cpp
  clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp
  clang/include/clang/Serialization/ASTWriter.h
  clang/include/clang/Serialization/PCHContainerOperations.h
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/lib/Serialization/PCHContainerOperations.cpp
  llvm/include/llvm/Bitcode/BitcodeWriter.h
  llvm/include/llvm/Bitstream/BitstreamWriter.h
  llvm/include/llvm/Remarks/BitstreamRemarkSerializer.h
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/ExecutionEngine/Orc/ThreadSafeModule.cpp
  llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
  llvm/tools/llvm-cat/llvm-cat.cpp
  llvm/tools/llvm-modextract/llvm-modextract.cpp
  llvm/unittests/Bitstream/BitstreamReaderTest.cpp
  llvm/unittests/Bitstream/BitstreamWriterTest.cpp

Index: llvm/unittests/Bitstream/BitstreamWriterTest.cpp
===
--- llvm/unittests/Bitstream/BitstreamWriterTest.cpp
+++ llvm/unittests/Bitstream/BitstreamWriterTest.cpp
@@ -8,27 +8,31 @@
 
 #include "llvm/Bitstream/BitstreamWriter.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallString.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+#include 
+
 using namespace llvm;
 
+using ::testing::IsEmpty;
+
 namespace {
 
 TEST(BitstreamWriterTest, emitBlob) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("str", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef("str\0", 4), Buffer);
+  EXPECT_EQ(StringRef("str\0", 4), StringRef(Buffer.data(), Buffer.size()));
 }
 
 TEST(BitstreamWriterTest, emitBlobWithSize) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   {
 BitstreamWriter W(Buffer);
 W.emitBlob("str");
   }
-  SmallString<64> Expected;
+  std::vector Expected;
   {
 BitstreamWriter W(Expected);
 W.EmitVBR(3, 6);
@@ -38,21 +42,21 @@
 W.Emit('r', 8);
 W.Emit(0, 8);
   }
-  EXPECT_EQ(StringRef(Expected), Buffer);
+  EXPECT_EQ(Expected, Buffer);
 }
 
 TEST(BitstreamWriterTest, emitBlobEmpty) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef(""), Buffer);
+  EXPECT_THAT(Buffer, IsEmpty());
 }
 
 TEST(BitstreamWriterTest, emitBlob4ByteAligned) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("str0", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef("str0"), Buffer);
+  EXPECT_EQ(StringRef("str0"), StringRef(Buffer.data(), Buffer.size()));
 }
 
 } // end namespace
Index: llvm/unittests/Bitstream/BitstreamReaderTest.cpp
===
--- llvm/unittests/Bitstream/BitstreamReaderTest.cpp
+++ llvm/unittests/Bitstream/BitstreamReaderTest.cpp
@@ -11,6 +11,8 @@
 #include "llvm/Bitstream/BitstreamWriter.h"
 #include "gtest/gtest.h"
 
+#include 
+
 using namespace llvm;
 
 namespace {
@@ -96,7 +98,7 @@
 StringRef BlobIn((const char *)BlobData.begin(), BlobSize);
 
 // Write the bitcode.
-SmallVector Buffer;
+std::vector Buffer;
 unsigned AbbrevID;
 {
   BitstreamWriter Stream(Buffer);
@@ -115,7 +117,7 @@
 
 // Stream the buffer into the reader.
 BitstreamCursor Stream(
-ArrayRef((const uint8_t *)Buffer.begin(), Buffer.size()));
+ArrayRef((const uint8_t *)Buffer.data(), Buffer.size()));
 
 // Header.  Included in test so that we can run llvm-bcanalyzer to debug
 // when there are problems.
Index: llvm/tools/llvm-modextract/llvm-modextract.cpp
===
--- llvm/tools/llvm-modextract/llvm-modextract.cpp
+++ llvm/tools/llvm-modextract/llvm-modextract.cpp
@@ -58,12 +58,12 @@
   ExitOnErr(errorCodeToError(EC));
 
   if (BinaryExtract) {
-SmallVector Result;
+std::vector Result;
 BitcodeWriter Writer(Result);
-Result.append(Ms[ModuleIndex].getBuffer().begin(),
+Result.insert(Result.end(), Ms[ModuleIndex].getBuffer().begin(),
   Ms[ModuleIndex].getBuffer().end());
 Writer.copyStrtab(Ms[ModuleIndex].getStrtab());
-Out->os() << Result;
+Out->os().write(Result.data(), Result.size());
 Out->keep();
 return 0;
   }
Index: llvm/tools/llvm-cat/llvm-cat.cpp
===
--- llvm/tools/llvm-cat/llvm-cat.cpp
+++ 

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

Requesting changes just to be sure we consider the other options.  I don't 
think it's good that `SmallVector` is no longer useful for large byte streams; 
I would prefer to fix that then stop using the type.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

This is thanks to a commit of mine that shaved a word off of `SmallVector`.  
Some options to consider:

1. Revert to word-size integers (`size_t`? `uintptr_t`?) for Size and Capacity 
for small-enough types.  Could be just if `sizeof(T)==1`.  Or maybe just for 
`char` and `unsigned char`.
2. Revert my patch entirely and go back to words (these used to be `void*`).
3. (Your patch, stop using `SmallVector`.)

I think I would prefer some variation of (1) over (3).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Andrew via Phabricator via cfe-commits]

browneee updated this revision to Diff 255856.
browneee added a comment.

Fix build errors. Missed -DLLVM_ENABLE_PROJECTS in previous local test builds.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621

Files:
  clang/include/clang/Serialization/ASTWriter.h
  clang/include/clang/Serialization/PCHContainerOperations.h
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/lib/Serialization/PCHContainerOperations.cpp
  llvm/include/llvm/Bitcode/BitcodeWriter.h
  llvm/include/llvm/Bitstream/BitstreamWriter.h
  llvm/include/llvm/Remarks/BitstreamRemarkSerializer.h
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/ExecutionEngine/Orc/ThreadSafeModule.cpp
  llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
  llvm/tools/llvm-cat/llvm-cat.cpp
  llvm/tools/llvm-modextract/llvm-modextract.cpp
  llvm/unittests/Bitstream/BitstreamReaderTest.cpp
  llvm/unittests/Bitstream/BitstreamWriterTest.cpp

Index: llvm/unittests/Bitstream/BitstreamWriterTest.cpp
===
--- llvm/unittests/Bitstream/BitstreamWriterTest.cpp
+++ llvm/unittests/Bitstream/BitstreamWriterTest.cpp
@@ -8,27 +8,31 @@
 
 #include "llvm/Bitstream/BitstreamWriter.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallString.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+#include 
+
 using namespace llvm;
 
+using ::testing::IsEmpty;
+
 namespace {
 
 TEST(BitstreamWriterTest, emitBlob) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("str", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef("str\0", 4), Buffer);
+  EXPECT_EQ(StringRef("str\0", 4), StringRef(Buffer.data(), Buffer.size()));
 }
 
 TEST(BitstreamWriterTest, emitBlobWithSize) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   {
 BitstreamWriter W(Buffer);
 W.emitBlob("str");
   }
-  SmallString<64> Expected;
+  std::vector Expected;
   {
 BitstreamWriter W(Expected);
 W.EmitVBR(3, 6);
@@ -38,21 +42,21 @@
 W.Emit('r', 8);
 W.Emit(0, 8);
   }
-  EXPECT_EQ(StringRef(Expected), Buffer);
+  EXPECT_EQ(Expected, Buffer);
 }
 
 TEST(BitstreamWriterTest, emitBlobEmpty) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef(""), Buffer);
+  EXPECT_THAT(Buffer, IsEmpty());
 }
 
 TEST(BitstreamWriterTest, emitBlob4ByteAligned) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("str0", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef("str0"), Buffer);
+  EXPECT_EQ(StringRef("str0"), StringRef(Buffer.data(), Buffer.size()));
 }
 
 } // end namespace
Index: llvm/unittests/Bitstream/BitstreamReaderTest.cpp
===
--- llvm/unittests/Bitstream/BitstreamReaderTest.cpp
+++ llvm/unittests/Bitstream/BitstreamReaderTest.cpp
@@ -11,6 +11,8 @@
 #include "llvm/Bitstream/BitstreamWriter.h"
 #include "gtest/gtest.h"
 
+#include 
+
 using namespace llvm;
 
 namespace {
@@ -96,7 +98,7 @@
 StringRef BlobIn((const char *)BlobData.begin(), BlobSize);
 
 // Write the bitcode.
-SmallVector Buffer;
+std::vector Buffer;
 unsigned AbbrevID;
 {
   BitstreamWriter Stream(Buffer);
@@ -115,7 +117,7 @@
 
 // Stream the buffer into the reader.
 BitstreamCursor Stream(
-ArrayRef((const uint8_t *)Buffer.begin(), Buffer.size()));
+ArrayRef((const uint8_t *)Buffer.data(), Buffer.size()));
 
 // Header.  Included in test so that we can run llvm-bcanalyzer to debug
 // when there are problems.
Index: llvm/tools/llvm-modextract/llvm-modextract.cpp
===
--- llvm/tools/llvm-modextract/llvm-modextract.cpp
+++ llvm/tools/llvm-modextract/llvm-modextract.cpp
@@ -58,12 +58,12 @@
   ExitOnErr(errorCodeToError(EC));
 
   if (BinaryExtract) {
-SmallVector Result;
+std::vector Result;
 BitcodeWriter Writer(Result);
-Result.append(Ms[ModuleIndex].getBuffer().begin(),
+Result.insert(Result.end(), Ms[ModuleIndex].getBuffer().begin(),
   Ms[ModuleIndex].getBuffer().end());
 Writer.copyStrtab(Ms[ModuleIndex].getStrtab());
-Out->os() << Result;
+Out->os().write(Result.data(), Result.size());
 Out->keep();
 return 0;
   }
Index: llvm/tools/llvm-cat/llvm-cat.cpp
===
--- llvm/tools/llvm-cat/llvm-cat.cpp
+++ llvm/tools/llvm-cat/llvm-cat.cpp
@@ -54,7 +54,7 @@
   ExitOnError ExitOnErr("llvm-cat: ");
   LLVMContext Context;
 
-  SmallVector Buffer;
+  

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Andrew via Phabricator via cfe-commits]

browneee updated this revision to Diff 255831.
browneee added a comment.

Fix more build errors.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621

Files:
  clang/include/clang/Serialization/ASTWriter.h
  clang/include/clang/Serialization/PCHContainerOperations.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  llvm/include/llvm/Bitcode/BitcodeWriter.h
  llvm/include/llvm/Bitstream/BitstreamWriter.h
  llvm/include/llvm/Remarks/BitstreamRemarkSerializer.h
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/ExecutionEngine/Orc/ThreadSafeModule.cpp
  llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
  llvm/tools/llvm-cat/llvm-cat.cpp
  llvm/tools/llvm-modextract/llvm-modextract.cpp
  llvm/unittests/Bitstream/BitstreamReaderTest.cpp
  llvm/unittests/Bitstream/BitstreamWriterTest.cpp

Index: llvm/unittests/Bitstream/BitstreamWriterTest.cpp
===
--- llvm/unittests/Bitstream/BitstreamWriterTest.cpp
+++ llvm/unittests/Bitstream/BitstreamWriterTest.cpp
@@ -8,27 +8,31 @@
 
 #include "llvm/Bitstream/BitstreamWriter.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallString.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+#include 
+
 using namespace llvm;
 
+using ::testing::IsEmpty;
+
 namespace {
 
 TEST(BitstreamWriterTest, emitBlob) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("str", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef("str\0", 4), Buffer);
+  EXPECT_EQ(StringRef("str\0", 4), StringRef(Buffer.data(), Buffer.size()));
 }
 
 TEST(BitstreamWriterTest, emitBlobWithSize) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   {
 BitstreamWriter W(Buffer);
 W.emitBlob("str");
   }
-  SmallString<64> Expected;
+  std::vector Expected;
   {
 BitstreamWriter W(Expected);
 W.EmitVBR(3, 6);
@@ -38,21 +42,21 @@
 W.Emit('r', 8);
 W.Emit(0, 8);
   }
-  EXPECT_EQ(StringRef(Expected), Buffer);
+  EXPECT_EQ(Expected, Buffer);
 }
 
 TEST(BitstreamWriterTest, emitBlobEmpty) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef(""), Buffer);
+  EXPECT_THAT(Buffer, IsEmpty());
 }
 
 TEST(BitstreamWriterTest, emitBlob4ByteAligned) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("str0", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef("str0"), Buffer);
+  EXPECT_EQ(StringRef("str0"), StringRef(Buffer.data(), Buffer.size()));
 }
 
 } // end namespace
Index: llvm/unittests/Bitstream/BitstreamReaderTest.cpp
===
--- llvm/unittests/Bitstream/BitstreamReaderTest.cpp
+++ llvm/unittests/Bitstream/BitstreamReaderTest.cpp
@@ -11,6 +11,8 @@
 #include "llvm/Bitstream/BitstreamWriter.h"
 #include "gtest/gtest.h"
 
+#include 
+
 using namespace llvm;
 
 namespace {
@@ -96,7 +98,7 @@
 StringRef BlobIn((const char *)BlobData.begin(), BlobSize);
 
 // Write the bitcode.
-SmallVector Buffer;
+std::vector Buffer;
 unsigned AbbrevID;
 {
   BitstreamWriter Stream(Buffer);
@@ -115,7 +117,7 @@
 
 // Stream the buffer into the reader.
 BitstreamCursor Stream(
-ArrayRef((const uint8_t *)Buffer.begin(), Buffer.size()));
+ArrayRef((const uint8_t *)Buffer.data(), Buffer.size()));
 
 // Header.  Included in test so that we can run llvm-bcanalyzer to debug
 // when there are problems.
Index: llvm/tools/llvm-modextract/llvm-modextract.cpp
===
--- llvm/tools/llvm-modextract/llvm-modextract.cpp
+++ llvm/tools/llvm-modextract/llvm-modextract.cpp
@@ -58,12 +58,12 @@
   ExitOnErr(errorCodeToError(EC));
 
   if (BinaryExtract) {
-SmallVector Result;
+std::vector Result;
 BitcodeWriter Writer(Result);
-Result.append(Ms[ModuleIndex].getBuffer().begin(),
+Result.insert(Result.end(), Ms[ModuleIndex].getBuffer().begin(),
   Ms[ModuleIndex].getBuffer().end());
 Writer.copyStrtab(Ms[ModuleIndex].getStrtab());
-Out->os() << Result;
+Out->os().write(Result.data(), Result.size());
 Out->keep();
 return 0;
   }
Index: llvm/tools/llvm-cat/llvm-cat.cpp
===
--- llvm/tools/llvm-cat/llvm-cat.cpp
+++ llvm/tools/llvm-cat/llvm-cat.cpp
@@ -54,7 +54,7 @@
   ExitOnError ExitOnErr("llvm-cat: ");
   LLVMContext Context;
 
-  SmallVector Buffer;
+  std::vector Buffer;
   BitcodeWriter Writer(Buffer);
   if (BinaryCat) {
 for (const auto  : InputFilenames) {
Index: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

[Andrew via Phabricator via cfe-commits]

browneee updated this revision to Diff 255773.
browneee marked an inline comment as done.
browneee added a comment.
Herald added subscribers: cfe-commits, arphaman.
Herald added a project: clang.

Fix build.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77621/new/

https://reviews.llvm.org/D77621

Files:
  clang/include/clang/Serialization/ASTWriter.h
  clang/include/clang/Serialization/PCHContainerOperations.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  llvm/include/llvm/Bitcode/BitcodeWriter.h
  llvm/include/llvm/Bitstream/BitstreamWriter.h
  llvm/include/llvm/Remarks/BitstreamRemarkSerializer.h
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/ExecutionEngine/Orc/ThreadSafeModule.cpp
  llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
  llvm/tools/llvm-cat/llvm-cat.cpp
  llvm/tools/llvm-modextract/llvm-modextract.cpp
  llvm/unittests/Bitstream/BitstreamReaderTest.cpp
  llvm/unittests/Bitstream/BitstreamWriterTest.cpp

Index: llvm/unittests/Bitstream/BitstreamWriterTest.cpp
===
--- llvm/unittests/Bitstream/BitstreamWriterTest.cpp
+++ llvm/unittests/Bitstream/BitstreamWriterTest.cpp
@@ -8,27 +8,31 @@
 
 #include "llvm/Bitstream/BitstreamWriter.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallString.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+#include 
+
 using namespace llvm;
 
+using ::testing::IsEmpty;
+
 namespace {
 
 TEST(BitstreamWriterTest, emitBlob) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("str", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef("str\0", 4), Buffer);
+  EXPECT_EQ(StringRef("str\0", 4), StringRef(Buffer.data(), Buffer.size()));
 }
 
 TEST(BitstreamWriterTest, emitBlobWithSize) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   {
 BitstreamWriter W(Buffer);
 W.emitBlob("str");
   }
-  SmallString<64> Expected;
+  std::vector Expected;
   {
 BitstreamWriter W(Expected);
 W.EmitVBR(3, 6);
@@ -38,21 +42,21 @@
 W.Emit('r', 8);
 W.Emit(0, 8);
   }
-  EXPECT_EQ(StringRef(Expected), Buffer);
+  EXPECT_EQ(Expected, Buffer);
 }
 
 TEST(BitstreamWriterTest, emitBlobEmpty) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef(""), Buffer);
+  EXPECT_THAT(Buffer, IsEmpty());
 }
 
 TEST(BitstreamWriterTest, emitBlob4ByteAligned) {
-  SmallString<64> Buffer;
+  std::vector Buffer;
   BitstreamWriter W(Buffer);
   W.emitBlob("str0", /* ShouldEmitSize */ false);
-  EXPECT_EQ(StringRef("str0"), Buffer);
+  EXPECT_EQ(StringRef("str0"), StringRef(Buffer.data(), Buffer.size()));
 }
 
 } // end namespace
Index: llvm/unittests/Bitstream/BitstreamReaderTest.cpp
===
--- llvm/unittests/Bitstream/BitstreamReaderTest.cpp
+++ llvm/unittests/Bitstream/BitstreamReaderTest.cpp
@@ -11,6 +11,8 @@
 #include "llvm/Bitstream/BitstreamWriter.h"
 #include "gtest/gtest.h"
 
+#include 
+
 using namespace llvm;
 
 namespace {
@@ -96,7 +98,7 @@
 StringRef BlobIn((const char *)BlobData.begin(), BlobSize);
 
 // Write the bitcode.
-SmallVector Buffer;
+std::vector Buffer;
 unsigned AbbrevID;
 {
   BitstreamWriter Stream(Buffer);
@@ -115,7 +117,7 @@
 
 // Stream the buffer into the reader.
 BitstreamCursor Stream(
-ArrayRef((const uint8_t *)Buffer.begin(), Buffer.size()));
+ArrayRef((const uint8_t *)Buffer.data(), Buffer.size()));
 
 // Header.  Included in test so that we can run llvm-bcanalyzer to debug
 // when there are problems.
Index: llvm/tools/llvm-modextract/llvm-modextract.cpp
===
--- llvm/tools/llvm-modextract/llvm-modextract.cpp
+++ llvm/tools/llvm-modextract/llvm-modextract.cpp
@@ -58,12 +58,12 @@
   ExitOnErr(errorCodeToError(EC));
 
   if (BinaryExtract) {
-SmallVector Result;
+std::vector Result;
 BitcodeWriter Writer(Result);
-Result.append(Ms[ModuleIndex].getBuffer().begin(),
+Result.insert(Result.end(), Ms[ModuleIndex].getBuffer().begin(),
   Ms[ModuleIndex].getBuffer().end());
 Writer.copyStrtab(Ms[ModuleIndex].getStrtab());
-Out->os() << Result;
+Out->os().write(Result.data(), Result.size());
 Out->keep();
 return 0;
   }
Index: llvm/tools/llvm-cat/llvm-cat.cpp
===
--- llvm/tools/llvm-cat/llvm-cat.cpp
+++ llvm/tools/llvm-cat/llvm-cat.cpp
@@ -54,7 +54,7 @@
   ExitOnError ExitOnErr("llvm-cat: ");
   LLVMContext Context;
 
-  SmallVector Buffer;
+  std::vector Buffer;
   BitcodeWriter Writer(Buffer);
   if (BinaryCat) {
 for (const auto  :