Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Wed, Jun 23, 2021 at 05:43:32PM -0600, Martin Sebor wrote: > On 6/22/21 11:23 PM, Trevor Saunders wrote: > > On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: > > > On 6/21/21 1:15 AM, Richard Biener wrote: > > > > On Fri, Jun 18, 2021 at 6:03 PM Martin Sebor wrote: > > > > > > > > > > On 6/18/21 4:38 AM, Richard Biener wrote: > > > > > > On Thu, Jun 17, 2021 at 4:43 PM Martin Sebor > > > > > > wrote: > > > > > > > > > > > > > > On 6/17/21 12:03 AM, Richard Biener wrote: > > > > > > > > On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote: > > > > > > > > > > Richard Biener via Gcc-patches > > > > > > > > > > writes: > > > > > > > > > > > On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > This makes it clear the caller owns the vector, and > > > > > > > > > > > > ensures it is cleaned up. > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Trevor Saunders > > > > > > > > > > > > > > > > > > > > > > > > bootstrapped and regtested on x86_64-linux-gnu, ok? > > > > > > > > > > > > > > > > > > > > > > OK. > > > > > > > > > > > > > > > > > > > > > > Btw, are "standard API" returns places we can use 'auto'? > > > > > > > > > > > That would avoid > > > > > > > > > > > excessive indent for > > > > > > > > > > > > > > > > > > > > > > - dom_bbs = get_dominated_by_region (CDI_DOMINATORS, > > > > > > > > > > > -bbs.address (), > > > > > > > > > > > -bbs.length ()); > > > > > > > > > > > + auto_vec dom_bbs = > > > > > > > > > > > get_dominated_by_region (CDI_DOMINATORS, > > > > > > > > > > > + > > > > > > > > > > > bbs.address (), > > > > > > > > > > > + > > > > > > > > > > > bbs.length ()); > > > > > > > > > > > > > > > > > > > > > > and just uses > > > > > > > > > > > > > > > > > > > > > >auto dom_bbs = get_dominated_by_region (... > > > > > > > > > > > > > > > > > > > > > > Not asking you to do this, just a question for the > > > > > > > > > > > audience. > > > > > > > > > > > > > > > > > > > > Personally I think this would be surprising for something > > > > > > > > > > that doesn't > > > > > > > > > > have copy semantics. (Not that I'm trying to reopen that > > > > > > > > > > debate here :-) > > > > > > > > > > FWIW, I agree not having copy semantics is probably the > > > > > > > > > > most practical > > > > > > > > > > way forward for now.) > > > > > > > > > > > > > > > > > > But you did open the door for me to reiterate my strong > > > > > > > > > disagreement > > > > > > > > > with that. The best C++ practice going back to the early > > > > > > > > > 1990's is > > > > > > > > > to make types safely copyable and assignable. It is the > > > > > > > > > default for > > > > > > > > > all types, in both C++ and C, and so natural and expected. > > > > > > > > > > > > > > > > > > Preventing copying is appropriate in special and rare > > > > > > > > > circumstances > > > > > > > > > (e.g, a mutex may not be copyable, or a file or iostream > > > > > > > > > object may > > > > > > > > > not be because they represent a unique physical resource.) > > > > > > > > > > > > > > > > > > In the absence of such special circumstances preventing > > > > > > > > > copying is > > > > > > > > > unexpected, and in the case of an essential building block > > > > > > > > > such as > > > > > > > > > a container, makes the type difficult to use. > > > > > > > > > > > > > > > > > > The only argument for disabling copying that has been given is > > > > > > > > > that it could be surprising(*). But because all types are > > > > > > > > > copyable > > > > > > > > > by default the "surprise" is usually when one can't be. > > > > > > > > > > > > > > > > > > I think Richi's "surprising" has to do with the fact that it > > > > > > > > > lets > > > > > > > > > one inadvertently copy a large amount of data, thus leading to > > > > > > > > > an inefficiency. But by analogy, there are infinitely many > > > > > > > > > ways > > > > > > > > > to end up with inefficient code (e.g., deep recursion, or heap > > > > > > > > > allocation in a loop), and they are not a reason to ban the > > > > > > > > > coding > > > > > > > > > constructs that might lead to it. > > > > > > > > > > > > > > > > > > IIUC, Jason's comment about surprising effects was about > > > > > > > > > implicit > > > > > > > > > conversion from auto_vec to vec. I share that concern, and > > > > > > > > > agree > > > > > > > > > that it should be addressed by preventing the conversion (as > > > > > > > > > Jason > > > > > > > > > suggested). > > > > > > > > > > > > > > > > But fact is that how vec<> and auto_vec<> are used today in G
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Thu, Jun 24, 2021 at 6:28 PM Richard Sandiford wrote: > > Richard Biener via Gcc-patches writes: > > On Wed, Jun 23, 2021 at 12:22 PM Richard Sandiford > > wrote: > >> > >> Richard Biener writes: > >> > On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders > >> > wrote: > >> >> > >> >> On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: > >> >> > On 6/21/21 1:15 AM, Richard Biener wrote: > >> > [...] > >> >> > > > >> >> > > But maybe I'm misunderstanding C++ too much :/ > >> >> > > > >> >> > > Well, I guess b) from above means auto_vec<> passing to > >> >> > > vec<> taking functions will need changes? > >> >> > > >> >> > Converting an auto_vec object to a vec slices off its data members. > >> >> > The auto_vec specialization has no data members so that's not > >> >> > a bug in and of itself, but auto_vec does have data members > >> >> > so that would be a bug. The risk is not just passing it to > >> >> > functions by value but also returning it. That risk was made > >> >> > worse by the addition of the move ctor. > >> >> > >> >> I would agree that the conversion from auto_vec<> to vec<> is > >> >> questionable, and should get some work at some point, perhaps just > >> >> passingauto_vec references is good enough, or perhaps there is value in > >> >> some const_vec view to avoid having to rely on optimizations, I'm not > >> >> sure without looking more at the usage. > >> > > >> > We do need to be able to provide APIs that work with both auto_vec<> > >> > and vec<>, I agree that those currently taking a vec<> by value are > >> > fragile (and we've had bugs there before), but I'm not ready to say > >> > that changing them all to [const] vec<>& is OK. The alternative > >> > would be passing a const_vec<> by value, passing that along to > >> > const vec<>& APIs should be valid then (I can see quite some API > >> > boundary cleanups being necessary here ...). > >> > >> FWIW, as far as const_vec<> goes, we already have array_slice, which is > >> mostly a version of std::span. (The only real reason for not using > >> std::span itself is that it requires a later version of C++.) > >> > >> Taking those as arguments has the advantage that we can pass normal > >> arrays as well. > > > > It's not really a "const" thing it seems though it certainly would not > > expose > > any API that would reallocate the vector (which is the problematic part > > of passing vec<> by value, not necessarily changing elements in-place). > > > > Since array_slice doesn't inherit most of the vec<> API transforming an > > API from vec<> to array_slice<> will be also quite some work. But I > > agree it might be useful for generic API stuff. > > Which API functions would be the most useful ones to have? We could > add them if necessary. I think we'll see when introducing uses. I guess that vec<> to const vec<>& changes will be mostly fine but for vec<> to vec<>& I might prefer vec<> to array_slice<> since that makes clear the caller won't re-allocate. We'll see what those APIs would require then. > There again, for things like searching and sorting, I think it would > be better to use where possible. It should usually be > more efficient than the void* callback approach that the C code tended > to use. Not sure whether really is better, we've specifically replaced libc qsort calls with our own sort to avoid all sorts of host isues and the vec<>::bsearch is inline and thus the callbacks can be inlined. Richard. > Thanks, > Richard
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
Richard Biener via Gcc-patches writes: > On Wed, Jun 23, 2021 at 12:22 PM Richard Sandiford > wrote: >> >> Richard Biener writes: >> > On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders >> > wrote: >> >> >> >> On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: >> >> > On 6/21/21 1:15 AM, Richard Biener wrote: >> > [...] >> >> > > >> >> > > But maybe I'm misunderstanding C++ too much :/ >> >> > > >> >> > > Well, I guess b) from above means auto_vec<> passing to >> >> > > vec<> taking functions will need changes? >> >> > >> >> > Converting an auto_vec object to a vec slices off its data members. >> >> > The auto_vec specialization has no data members so that's not >> >> > a bug in and of itself, but auto_vec does have data members >> >> > so that would be a bug. The risk is not just passing it to >> >> > functions by value but also returning it. That risk was made >> >> > worse by the addition of the move ctor. >> >> >> >> I would agree that the conversion from auto_vec<> to vec<> is >> >> questionable, and should get some work at some point, perhaps just >> >> passingauto_vec references is good enough, or perhaps there is value in >> >> some const_vec view to avoid having to rely on optimizations, I'm not >> >> sure without looking more at the usage. >> > >> > We do need to be able to provide APIs that work with both auto_vec<> >> > and vec<>, I agree that those currently taking a vec<> by value are >> > fragile (and we've had bugs there before), but I'm not ready to say >> > that changing them all to [const] vec<>& is OK. The alternative >> > would be passing a const_vec<> by value, passing that along to >> > const vec<>& APIs should be valid then (I can see quite some API >> > boundary cleanups being necessary here ...). >> >> FWIW, as far as const_vec<> goes, we already have array_slice, which is >> mostly a version of std::span. (The only real reason for not using >> std::span itself is that it requires a later version of C++.) >> >> Taking those as arguments has the advantage that we can pass normal >> arrays as well. > > It's not really a "const" thing it seems though it certainly would not expose > any API that would reallocate the vector (which is the problematic part > of passing vec<> by value, not necessarily changing elements in-place). > > Since array_slice doesn't inherit most of the vec<> API transforming an > API from vec<> to array_slice<> will be also quite some work. But I > agree it might be useful for generic API stuff. Which API functions would be the most useful ones to have? We could add them if necessary. There again, for things like searching and sorting, I think it would be better to use where possible. It should usually be more efficient than the void* callback approach that the C code tended to use. Thanks, Richard
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On 6/24/21 3:27 AM, Richard Biener wrote: On Thu, Jun 24, 2021 at 12:56 AM Martin Sebor wrote: On 6/23/21 1:43 AM, Richard Biener wrote: On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders wrote: On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: On 6/21/21 1:15 AM, Richard Biener wrote: [...] But maybe I'm misunderstanding C++ too much :/ Well, I guess b) from above means auto_vec<> passing to vec<> taking functions will need changes? Converting an auto_vec object to a vec slices off its data members. The auto_vec specialization has no data members so that's not a bug in and of itself, but auto_vec does have data members so that would be a bug. The risk is not just passing it to functions by value but also returning it. That risk was made worse by the addition of the move ctor. I would agree that the conversion from auto_vec<> to vec<> is questionable, and should get some work at some point, perhaps just passingauto_vec references is good enough, or perhaps there is value in some const_vec view to avoid having to rely on optimizations, I'm not sure without looking more at the usage. We do need to be able to provide APIs that work with both auto_vec<> and vec<>, I agree that those currently taking a vec<> by value are fragile (and we've had bugs there before), but I'm not ready to say that changing them all to [const] vec<>& is OK. The alternative would be passing a const_vec<> by value, passing that along to const vec<>& APIs should be valid then (I can see quite some API boundary cleanups being necessary here ...). But with all this I don't know how to adjust auto_vec<> to no longer "decay" to vec<> but still being able to pass it to vec<>& and being able to call vec<> member functions w/o jumping through hoops. Any hints on that? private inheritance achieves the first but also hides all the API ... The simplest way to do that is by preventing the implicit conversion between the two types and adding a to_vec() member function to auto_vec as Jason suggested. This exposes the implicit conversions to the base vec, forcing us to review each and "fix" it either by changing the argument to either vec& or const vec&, or less often to avoid the indirection if necessary, by converting the auto_vec to vec explicitly by calling to_vec(). The attached diff shows a very rough POC of what that might look like. A side benefit is that it improves const-safety and makes the effects of functions on their callers easier to understand. But that's orthogonal to making auto_vec copy constructible and copy assignable. That change can be made independently and with much less effort and no risk. There's a bunch of stuff I can't review because of lack of C++ knowledge (the vNULL changes). I suppose that + /* Prevent implicit conversion from auto_vec. Use auto_vec::to_vec() + instead. */ + template + vec (auto_vec &) = delete; + + template + void operator= (auto_vec &) = delete; still allows passing auto_vec<> to [const] vec<> & without the .to_vec so it's just the by-value passing that becomes explicit? If so then I agree this is an improvement. The patch is likely incomplete though or is it really only so few signatures that need changing? Yes, to both questions. I just wanted to show how we might go about making this transition. I converted a few files but many more remain. I stopped when changing a vec argument to const vec& started to cause errors due to missing const down the line (e.g., assigning the address of a vec element to an uqualified pointer). I'll need to follow where that pointer flows and see if it's used to modify the object or if it too could be made const. To me that seems worthwhile doing now but it makes progress slow. A separate question is whether the vec value arguments to functions that modify them should be changed to references or pointers. Both kinds of APIs exist but the latter seems prevalent. Changing them to the latter means more churn. Martin
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Thu, Jun 24, 2021 at 12:56 AM Martin Sebor wrote: > > On 6/23/21 1:43 AM, Richard Biener wrote: > > On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders > > wrote: > >> > >> On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: > >>> On 6/21/21 1:15 AM, Richard Biener wrote: > > [...] > > But maybe I'm misunderstanding C++ too much :/ > > Well, I guess b) from above means auto_vec<> passing to > vec<> taking functions will need changes? > >>> > >>> Converting an auto_vec object to a vec slices off its data members. > >>> The auto_vec specialization has no data members so that's not > >>> a bug in and of itself, but auto_vec does have data members > >>> so that would be a bug. The risk is not just passing it to > >>> functions by value but also returning it. That risk was made > >>> worse by the addition of the move ctor. > >> > >> I would agree that the conversion from auto_vec<> to vec<> is > >> questionable, and should get some work at some point, perhaps just > >> passingauto_vec references is good enough, or perhaps there is value in > >> some const_vec view to avoid having to rely on optimizations, I'm not > >> sure without looking more at the usage. > > > > We do need to be able to provide APIs that work with both auto_vec<> > > and vec<>, I agree that those currently taking a vec<> by value are > > fragile (and we've had bugs there before), but I'm not ready to say > > that changing them all to [const] vec<>& is OK. The alternative > > would be passing a const_vec<> by value, passing that along to > > const vec<>& APIs should be valid then (I can see quite some API > > boundary cleanups being necessary here ...). > > > > But with all this I don't know how to adjust auto_vec<> to no > > longer "decay" to vec<> but still being able to pass it to vec<>& > > and being able to call vec<> member functions w/o jumping through > > hoops. Any hints on that? private inheritance achieves the first > > but also hides all the API ... > > The simplest way to do that is by preventing the implicit conversion > between the two types and adding a to_vec() member function to > auto_vec as Jason suggested. This exposes the implicit conversions > to the base vec, forcing us to review each and "fix" it either by > changing the argument to either vec& or const vec&, or less often > to avoid the indirection if necessary, by converting the auto_vec > to vec explicitly by calling to_vec(). The attached diff shows > a very rough POC of what that might look like. A side benefit > is that it improves const-safety and makes the effects of functions > on their callers easier to understand. > > But that's orthogonal to making auto_vec copy constructible and copy > assignable. That change can be made independently and with much less > effort and no risk. There's a bunch of stuff I can't review because of lack of C++ knowledge (the vNULL changes). I suppose that + /* Prevent implicit conversion from auto_vec. Use auto_vec::to_vec() + instead. */ + template + vec (auto_vec &) = delete; + + template + void operator= (auto_vec &) = delete; still allows passing auto_vec<> to [const] vec<> & without the .to_vec so it's just the by-value passing that becomes explicit? If so then I agree this is an improvement. The patch is likely incomplete though or is it really only so few signatures that need changing? Thanks, Richard. > Martin
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Wed, Jun 23, 2021 at 12:22 PM Richard Sandiford wrote: > > Richard Biener writes: > > On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders > > wrote: > >> > >> On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: > >> > On 6/21/21 1:15 AM, Richard Biener wrote: > > [...] > >> > > > >> > > But maybe I'm misunderstanding C++ too much :/ > >> > > > >> > > Well, I guess b) from above means auto_vec<> passing to > >> > > vec<> taking functions will need changes? > >> > > >> > Converting an auto_vec object to a vec slices off its data members. > >> > The auto_vec specialization has no data members so that's not > >> > a bug in and of itself, but auto_vec does have data members > >> > so that would be a bug. The risk is not just passing it to > >> > functions by value but also returning it. That risk was made > >> > worse by the addition of the move ctor. > >> > >> I would agree that the conversion from auto_vec<> to vec<> is > >> questionable, and should get some work at some point, perhaps just > >> passingauto_vec references is good enough, or perhaps there is value in > >> some const_vec view to avoid having to rely on optimizations, I'm not > >> sure without looking more at the usage. > > > > We do need to be able to provide APIs that work with both auto_vec<> > > and vec<>, I agree that those currently taking a vec<> by value are > > fragile (and we've had bugs there before), but I'm not ready to say > > that changing them all to [const] vec<>& is OK. The alternative > > would be passing a const_vec<> by value, passing that along to > > const vec<>& APIs should be valid then (I can see quite some API > > boundary cleanups being necessary here ...). > > FWIW, as far as const_vec<> goes, we already have array_slice, which is > mostly a version of std::span. (The only real reason for not using > std::span itself is that it requires a later version of C++.) > > Taking those as arguments has the advantage that we can pass normal > arrays as well. It's not really a "const" thing it seems though it certainly would not expose any API that would reallocate the vector (which is the problematic part of passing vec<> by value, not necessarily changing elements in-place). Since array_slice doesn't inherit most of the vec<> API transforming an API from vec<> to array_slice<> will be also quite some work. But I agree it might be useful for generic API stuff. Richard. > Thanks, > Richard
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On 6/22/21 11:23 PM, Trevor Saunders wrote: On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: On 6/21/21 1:15 AM, Richard Biener wrote: On Fri, Jun 18, 2021 at 6:03 PM Martin Sebor wrote: On 6/18/21 4:38 AM, Richard Biener wrote: On Thu, Jun 17, 2021 at 4:43 PM Martin Sebor wrote: On 6/17/21 12:03 AM, Richard Biener wrote: On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor wrote: On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote: Richard Biener via Gcc-patches writes: On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders wrote: This makes it clear the caller owns the vector, and ensures it is cleaned up. Signed-off-by: Trevor Saunders bootstrapped and regtested on x86_64-linux-gnu, ok? OK. Btw, are "standard API" returns places we can use 'auto'? That would avoid excessive indent for - dom_bbs = get_dominated_by_region (CDI_DOMINATORS, -bbs.address (), -bbs.length ()); + auto_vec dom_bbs = get_dominated_by_region (CDI_DOMINATORS, + bbs.address (), + bbs.length ()); and just uses auto dom_bbs = get_dominated_by_region (... Not asking you to do this, just a question for the audience. Personally I think this would be surprising for something that doesn't have copy semantics. (Not that I'm trying to reopen that debate here :-) FWIW, I agree not having copy semantics is probably the most practical way forward for now.) But you did open the door for me to reiterate my strong disagreement with that. The best C++ practice going back to the early 1990's is to make types safely copyable and assignable. It is the default for all types, in both C++ and C, and so natural and expected. Preventing copying is appropriate in special and rare circumstances (e.g, a mutex may not be copyable, or a file or iostream object may not be because they represent a unique physical resource.) In the absence of such special circumstances preventing copying is unexpected, and in the case of an essential building block such as a container, makes the type difficult to use. The only argument for disabling copying that has been given is that it could be surprising(*). But because all types are copyable by default the "surprise" is usually when one can't be. I think Richi's "surprising" has to do with the fact that it lets one inadvertently copy a large amount of data, thus leading to an inefficiency. But by analogy, there are infinitely many ways to end up with inefficient code (e.g., deep recursion, or heap allocation in a loop), and they are not a reason to ban the coding constructs that might lead to it. IIUC, Jason's comment about surprising effects was about implicit conversion from auto_vec to vec. I share that concern, and agree that it should be addressed by preventing the conversion (as Jason suggested). But fact is that how vec<> and auto_vec<> are used today in GCC do not favor that. In fact your proposed vec<> would be quite radically different (and IMHO vec<> and auto_vec<> should be unified then to form your proposed new container). auto_vec<> at the moment simply maintains ownership like a smart pointer - which is _also_ not copyable. Yes, as we discussed in the review below, vec is not a good model because (as you note again above) it's constrained by its legacy uses. The best I think we can do for it is to make it safer to use. https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html Which is what Trevors patches do by simply disallowing things that do not work at the moment. (Smart pointers don't rule out copying. A std::unique_ptr does and std::shared_ptr doesn't. But vec and especially auto_vec are designed to be containers, not "unique pointers" so any relationship there is purely superficial and a distraction.) That auto_vec and vec share a name and an is-a relationship is incidental, an implementation detail leaked into the API. A better name than vector is hard to come up with, but the public inheritance is a design flaw, a bug waiting to be introduced due to the conversion and the assumptions the base vec makes about POD-ness and shallow copying. Hindsight is always 20/20 but past mistakes should not dictate the design of a general purpose vector-like container in GCC. That auto_vec<> "decays" to vec<> was on purpose design. By-value passing of vec<> is also on purpose to avoid an extra pointer indirection on each access. I think you may have misunderstood what I mean by is-a relationship. It's fine to convert an auto_vec to another interface. The danger is in allowing that to happen implicitly because that tends to let it happen even when it's not intended. The usual way to avoid that risk is to provide a conversion function, like auto_vec::to_vec(). This is also why standard classes like std::vector or std::string don't allow such implic
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On 6/23/21 1:43 AM, Richard Biener wrote:
On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders wrote:
On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote:
On 6/21/21 1:15 AM, Richard Biener wrote:
[...]
But maybe I'm misunderstanding C++ too much :/
Well, I guess b) from above means auto_vec<> passing to
vec<> taking functions will need changes?
Converting an auto_vec object to a vec slices off its data members.
The auto_vec specialization has no data members so that's not
a bug in and of itself, but auto_vec does have data members
so that would be a bug. The risk is not just passing it to
functions by value but also returning it. That risk was made
worse by the addition of the move ctor.
I would agree that the conversion from auto_vec<> to vec<> is
questionable, and should get some work at some point, perhaps just
passingauto_vec references is good enough, or perhaps there is value in
some const_vec view to avoid having to rely on optimizations, I'm not
sure without looking more at the usage.
We do need to be able to provide APIs that work with both auto_vec<>
and vec<>, I agree that those currently taking a vec<> by value are
fragile (and we've had bugs there before), but I'm not ready to say
that changing them all to [const] vec<>& is OK. The alternative
would be passing a const_vec<> by value, passing that along to
const vec<>& APIs should be valid then (I can see quite some API
boundary cleanups being necessary here ...).
But with all this I don't know how to adjust auto_vec<> to no
longer "decay" to vec<> but still being able to pass it to vec<>&
and being able to call vec<> member functions w/o jumping through
hoops. Any hints on that? private inheritance achieves the first
but also hides all the API ...
The simplest way to do that is by preventing the implicit conversion
between the two types and adding a to_vec() member function to
auto_vec as Jason suggested. This exposes the implicit conversions
to the base vec, forcing us to review each and "fix" it either by
changing the argument to either vec& or const vec&, or less often
to avoid the indirection if necessary, by converting the auto_vec
to vec explicitly by calling to_vec(). The attached diff shows
a very rough POC of what that might look like. A side benefit
is that it improves const-safety and makes the effects of functions
on their callers easier to understand.
But that's orthogonal to making auto_vec copy constructible and copy
assignable. That change can be made independently and with much less
effort and no risk.
Martin
diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index a671a3eb740..ab6db3860f5 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -759,8 +759,9 @@ extern tree c_finish_omp_clauses (tree, enum c_omp_region_type);
extern tree c_build_va_arg (location_t, tree, location_t, tree);
extern tree c_finish_transaction (location_t, tree, int);
extern bool c_tree_equal (tree, tree);
-extern tree c_build_function_call_vec (location_t, vec, tree,
- vec *, vec *);
+extern tree c_build_function_call_vec (location_t, const vec&,
+ tree, vec *,
+ vec *);
extern tree c_omp_clause_copy_ctor (tree, tree, tree);
/* Set to 0 at beginning of a function definition, set to 1 if
diff --git a/gcc/dominance.c b/gcc/dominance.c
index 6a262ce8283..cc63391a39a 100644
--- a/gcc/dominance.c
+++ b/gcc/dominance.c
@@ -1227,7 +1227,7 @@ recompute_dominator (enum cdi_direction dir, basic_block bb)
from BBS. */
static void
-prune_bbs_to_update_dominators (vec bbs,
+prune_bbs_to_update_dominators (vec &bbs,
bool conservative)
{
unsigned i;
@@ -1379,7 +1379,7 @@ determine_dominators_for_sons (struct graph *g, vec bbs,
a block of BBS in the current dominance tree dominate it. */
void
-iterate_fix_dominators (enum cdi_direction dir, vec bbs,
+iterate_fix_dominators (enum cdi_direction dir, vec &bbs,
bool conservative)
{
unsigned i;
diff --git a/gcc/dominance.h b/gcc/dominance.h
index 1a8c248ee98..970da02c594 100644
--- a/gcc/dominance.h
+++ b/gcc/dominance.h
@@ -78,7 +78,7 @@ checking_verify_dominators (cdi_direction dir)
basic_block recompute_dominator (enum cdi_direction, basic_block);
extern void iterate_fix_dominators (enum cdi_direction,
-vec , bool);
+vec &, bool);
extern void add_to_dominance_info (enum cdi_direction, basic_block);
extern void delete_from_dominance_info (enum cdi_direction, basic_block);
extern basic_block first_dom_son (enum cdi_direction, basic_block);
diff --git a/gcc/genautomata.c b/gcc/genautomata.c
index 6bbfc684afa..e488c5f28ef 100644
--- a/gcc/genautomata.c
+++ b/gcc/genautomata.c
@@ -6137,7 +6137,7 @@ evaluate_equiv_classes (automaton_t automaton, vec *equiv_classes)
/* The function merges equivalent states of AUTOMATON. */
static void
-merge_states (automaton_t automaton, vec equiv_classes)
+merge_states (automaton_t automaton, const vec &equiv_classes)
{
state_t curr_state;
state_t new_state;
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
Richard Biener writes: > On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders wrote: >> >> On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: >> > On 6/21/21 1:15 AM, Richard Biener wrote: > [...] >> > > >> > > But maybe I'm misunderstanding C++ too much :/ >> > > >> > > Well, I guess b) from above means auto_vec<> passing to >> > > vec<> taking functions will need changes? >> > >> > Converting an auto_vec object to a vec slices off its data members. >> > The auto_vec specialization has no data members so that's not >> > a bug in and of itself, but auto_vec does have data members >> > so that would be a bug. The risk is not just passing it to >> > functions by value but also returning it. That risk was made >> > worse by the addition of the move ctor. >> >> I would agree that the conversion from auto_vec<> to vec<> is >> questionable, and should get some work at some point, perhaps just >> passingauto_vec references is good enough, or perhaps there is value in >> some const_vec view to avoid having to rely on optimizations, I'm not >> sure without looking more at the usage. > > We do need to be able to provide APIs that work with both auto_vec<> > and vec<>, I agree that those currently taking a vec<> by value are > fragile (and we've had bugs there before), but I'm not ready to say > that changing them all to [const] vec<>& is OK. The alternative > would be passing a const_vec<> by value, passing that along to > const vec<>& APIs should be valid then (I can see quite some API > boundary cleanups being necessary here ...). FWIW, as far as const_vec<> goes, we already have array_slice, which is mostly a version of std::span. (The only real reason for not using std::span itself is that it requires a later version of C++.) Taking those as arguments has the advantage that we can pass normal arrays as well. Thanks, Richard
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders wrote: > > On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: > > On 6/21/21 1:15 AM, Richard Biener wrote: [...] > > > > > > But maybe I'm misunderstanding C++ too much :/ > > > > > > Well, I guess b) from above means auto_vec<> passing to > > > vec<> taking functions will need changes? > > > > Converting an auto_vec object to a vec slices off its data members. > > The auto_vec specialization has no data members so that's not > > a bug in and of itself, but auto_vec does have data members > > so that would be a bug. The risk is not just passing it to > > functions by value but also returning it. That risk was made > > worse by the addition of the move ctor. > > I would agree that the conversion from auto_vec<> to vec<> is > questionable, and should get some work at some point, perhaps just > passingauto_vec references is good enough, or perhaps there is value in > some const_vec view to avoid having to rely on optimizations, I'm not > sure without looking more at the usage. We do need to be able to provide APIs that work with both auto_vec<> and vec<>, I agree that those currently taking a vec<> by value are fragile (and we've had bugs there before), but I'm not ready to say that changing them all to [const] vec<>& is OK. The alternative would be passing a const_vec<> by value, passing that along to const vec<>& APIs should be valid then (I can see quite some API boundary cleanups being necessary here ...). But with all this I don't know how to adjust auto_vec<> to no longer "decay" to vec<> but still being able to pass it to vec<>& and being able to call vec<> member functions w/o jumping through hoops. Any hints on that? private inheritance achieves the first but also hides all the API ... > However I think that's a > separate issue and we can't and shouldn't fix everything at once. As > for slicing auto_vec I think that mostly "works" given the same > conditions as for auto_vec because the base that gets coppied is a > pointer to a valid embedded vec the same as in the source object. So as > long as the source outlives the copy, and the operations on either > object do not trigger a resize you get away with it, as much as it is > playing with fire. Returning a auto_vec sounds like a bug to > begin with, even if it worked correctly, but that aside, you can't > convert from auto_vec to auto_vec for n != M, so if the > function returns auto_vec you can implicitly convert to vec in the > caller, but you can't slice away part of the source object. Making more > things return auto_vec certainly increases the number of places > conversions to vec can take place and cause trouble, but you can't > fix everything at once, and its a preexisting issue, which would be the > same if the copy members were defined.
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: > On 6/21/21 1:15 AM, Richard Biener wrote: > > On Fri, Jun 18, 2021 at 6:03 PM Martin Sebor wrote: > > > > > > On 6/18/21 4:38 AM, Richard Biener wrote: > > > > On Thu, Jun 17, 2021 at 4:43 PM Martin Sebor wrote: > > > > > > > > > > On 6/17/21 12:03 AM, Richard Biener wrote: > > > > > > On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor > > > > > > wrote: > > > > > > > > > > > > > > On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote: > > > > > > > > Richard Biener via Gcc-patches writes: > > > > > > > > > On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > This makes it clear the caller owns the vector, and ensures > > > > > > > > > > it is cleaned up. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Trevor Saunders > > > > > > > > > > > > > > > > > > > > bootstrapped and regtested on x86_64-linux-gnu, ok? > > > > > > > > > > > > > > > > > > OK. > > > > > > > > > > > > > > > > > > Btw, are "standard API" returns places we can use 'auto'? > > > > > > > > > That would avoid > > > > > > > > > excessive indent for > > > > > > > > > > > > > > > > > > - dom_bbs = get_dominated_by_region (CDI_DOMINATORS, > > > > > > > > > -bbs.address (), > > > > > > > > > -bbs.length ()); > > > > > > > > > + auto_vec dom_bbs = get_dominated_by_region > > > > > > > > > (CDI_DOMINATORS, > > > > > > > > > + > > > > > > > > > bbs.address (), > > > > > > > > > + > > > > > > > > > bbs.length ()); > > > > > > > > > > > > > > > > > > and just uses > > > > > > > > > > > > > > > > > > auto dom_bbs = get_dominated_by_region (... > > > > > > > > > > > > > > > > > > Not asking you to do this, just a question for the audience. > > > > > > > > > > > > > > > > Personally I think this would be surprising for something that > > > > > > > > doesn't > > > > > > > > have copy semantics. (Not that I'm trying to reopen that > > > > > > > > debate here :-) > > > > > > > > FWIW, I agree not having copy semantics is probably the most > > > > > > > > practical > > > > > > > > way forward for now.) > > > > > > > > > > > > > > But you did open the door for me to reiterate my strong > > > > > > > disagreement > > > > > > > with that. The best C++ practice going back to the early 1990's > > > > > > > is > > > > > > > to make types safely copyable and assignable. It is the default > > > > > > > for > > > > > > > all types, in both C++ and C, and so natural and expected. > > > > > > > > > > > > > > Preventing copying is appropriate in special and rare > > > > > > > circumstances > > > > > > > (e.g, a mutex may not be copyable, or a file or iostream object > > > > > > > may > > > > > > > not be because they represent a unique physical resource.) > > > > > > > > > > > > > > In the absence of such special circumstances preventing copying is > > > > > > > unexpected, and in the case of an essential building block such as > > > > > > > a container, makes the type difficult to use. > > > > > > > > > > > > > > The only argument for disabling copying that has been given is > > > > > > > that it could be surprising(*). But because all types are > > > > > > > copyable > > > > > > > by default the "surprise" is usually when one can't be. > > > > > > > > > > > > > > I think Richi's "surprising" has to do with the fact that it lets > > > > > > > one inadvertently copy a large amount of data, thus leading to > > > > > > > an inefficiency. But by analogy, there are infinitely many ways > > > > > > > to end up with inefficient code (e.g., deep recursion, or heap > > > > > > > allocation in a loop), and they are not a reason to ban the coding > > > > > > > constructs that might lead to it. > > > > > > > > > > > > > > IIUC, Jason's comment about surprising effects was about implicit > > > > > > > conversion from auto_vec to vec. I share that concern, and agree > > > > > > > that it should be addressed by preventing the conversion (as Jason > > > > > > > suggested). > > > > > > > > > > > > But fact is that how vec<> and auto_vec<> are used today in GCC > > > > > > do not favor that. In fact your proposed vec<> would be quite > > > > > > radically > > > > > > different (and IMHO vec<> and auto_vec<> should be unified then to > > > > > > form your proposed new container). auto_vec<> at the moment simply > > > > > > maintains ownership like a smart pointer - which is _also_ not > > > > > > copyable. > > > > > > > > > > Yes, as we discussed in the review below, vec is not a good model > > > > > because (as you note again above) it's constrained by its legacy > > > > > uses. The best I think we can do for it is to make it safer to > > > > > use. > > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On 6/21/21 1:15 AM, Richard Biener wrote: On Fri, Jun 18, 2021 at 6:03 PM Martin Sebor wrote: On 6/18/21 4:38 AM, Richard Biener wrote: On Thu, Jun 17, 2021 at 4:43 PM Martin Sebor wrote: On 6/17/21 12:03 AM, Richard Biener wrote: On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor wrote: On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote: Richard Biener via Gcc-patches writes: On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders wrote: This makes it clear the caller owns the vector, and ensures it is cleaned up. Signed-off-by: Trevor Saunders bootstrapped and regtested on x86_64-linux-gnu, ok? OK. Btw, are "standard API" returns places we can use 'auto'? That would avoid excessive indent for - dom_bbs = get_dominated_by_region (CDI_DOMINATORS, -bbs.address (), -bbs.length ()); + auto_vec dom_bbs = get_dominated_by_region (CDI_DOMINATORS, + bbs.address (), + bbs.length ()); and just uses auto dom_bbs = get_dominated_by_region (... Not asking you to do this, just a question for the audience. Personally I think this would be surprising for something that doesn't have copy semantics. (Not that I'm trying to reopen that debate here :-) FWIW, I agree not having copy semantics is probably the most practical way forward for now.) But you did open the door for me to reiterate my strong disagreement with that. The best C++ practice going back to the early 1990's is to make types safely copyable and assignable. It is the default for all types, in both C++ and C, and so natural and expected. Preventing copying is appropriate in special and rare circumstances (e.g, a mutex may not be copyable, or a file or iostream object may not be because they represent a unique physical resource.) In the absence of such special circumstances preventing copying is unexpected, and in the case of an essential building block such as a container, makes the type difficult to use. The only argument for disabling copying that has been given is that it could be surprising(*). But because all types are copyable by default the "surprise" is usually when one can't be. I think Richi's "surprising" has to do with the fact that it lets one inadvertently copy a large amount of data, thus leading to an inefficiency. But by analogy, there are infinitely many ways to end up with inefficient code (e.g., deep recursion, or heap allocation in a loop), and they are not a reason to ban the coding constructs that might lead to it. IIUC, Jason's comment about surprising effects was about implicit conversion from auto_vec to vec. I share that concern, and agree that it should be addressed by preventing the conversion (as Jason suggested). But fact is that how vec<> and auto_vec<> are used today in GCC do not favor that. In fact your proposed vec<> would be quite radically different (and IMHO vec<> and auto_vec<> should be unified then to form your proposed new container). auto_vec<> at the moment simply maintains ownership like a smart pointer - which is _also_ not copyable. Yes, as we discussed in the review below, vec is not a good model because (as you note again above) it's constrained by its legacy uses. The best I think we can do for it is to make it safer to use. https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html Which is what Trevors patches do by simply disallowing things that do not work at the moment. (Smart pointers don't rule out copying. A std::unique_ptr does and std::shared_ptr doesn't. But vec and especially auto_vec are designed to be containers, not "unique pointers" so any relationship there is purely superficial and a distraction.) That auto_vec and vec share a name and an is-a relationship is incidental, an implementation detail leaked into the API. A better name than vector is hard to come up with, but the public inheritance is a design flaw, a bug waiting to be introduced due to the conversion and the assumptions the base vec makes about POD-ness and shallow copying. Hindsight is always 20/20 but past mistakes should not dictate the design of a general purpose vector-like container in GCC. That auto_vec<> "decays" to vec<> was on purpose design. By-value passing of vec<> is also on purpose to avoid an extra pointer indirection on each access. I think you may have misunderstood what I mean by is-a relationship. It's fine to convert an auto_vec to another interface. The danger is in allowing that to happen implicitly because that tends to let it happen even when it's not intended. The usual way to avoid that risk is to provide a conversion function, like auto_vec::to_vec(). This is also why standard classes like std::vector or std::string don't allow such implicit conversions and instead provide member functions (see for example Stroustrup: The C++ Programming Language
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Fri, Jun 18, 2021 at 6:03 PM Martin Sebor wrote: > > On 6/18/21 4:38 AM, Richard Biener wrote: > > On Thu, Jun 17, 2021 at 4:43 PM Martin Sebor wrote: > >> > >> On 6/17/21 12:03 AM, Richard Biener wrote: > >>> On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor wrote: > > On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote: > > Richard Biener via Gcc-patches writes: > >> On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders > >> wrote: > >>> > >>> This makes it clear the caller owns the vector, and ensures it is > >>> cleaned up. > >>> > >>> Signed-off-by: Trevor Saunders > >>> > >>> bootstrapped and regtested on x86_64-linux-gnu, ok? > >> > >> OK. > >> > >> Btw, are "standard API" returns places we can use 'auto'? That would > >> avoid > >> excessive indent for > >> > >> - dom_bbs = get_dominated_by_region (CDI_DOMINATORS, > >> -bbs.address (), > >> -bbs.length ()); > >> + auto_vec dom_bbs = get_dominated_by_region > >> (CDI_DOMINATORS, > >> + bbs.address > >> (), > >> + bbs.length > >> ()); > >> > >> and just uses > >> > >> auto dom_bbs = get_dominated_by_region (... > >> > >> Not asking you to do this, just a question for the audience. > > > > Personally I think this would be surprising for something that doesn't > > have copy semantics. (Not that I'm trying to reopen that debate here > > :-) > > FWIW, I agree not having copy semantics is probably the most practical > > way forward for now.) > > But you did open the door for me to reiterate my strong disagreement > with that. The best C++ practice going back to the early 1990's is > to make types safely copyable and assignable. It is the default for > all types, in both C++ and C, and so natural and expected. > > Preventing copying is appropriate in special and rare circumstances > (e.g, a mutex may not be copyable, or a file or iostream object may > not be because they represent a unique physical resource.) > > In the absence of such special circumstances preventing copying is > unexpected, and in the case of an essential building block such as > a container, makes the type difficult to use. > > The only argument for disabling copying that has been given is > that it could be surprising(*). But because all types are copyable > by default the "surprise" is usually when one can't be. > > I think Richi's "surprising" has to do with the fact that it lets > one inadvertently copy a large amount of data, thus leading to > an inefficiency. But by analogy, there are infinitely many ways > to end up with inefficient code (e.g., deep recursion, or heap > allocation in a loop), and they are not a reason to ban the coding > constructs that might lead to it. > > IIUC, Jason's comment about surprising effects was about implicit > conversion from auto_vec to vec. I share that concern, and agree > that it should be addressed by preventing the conversion (as Jason > suggested). > >>> > >>> But fact is that how vec<> and auto_vec<> are used today in GCC > >>> do not favor that. In fact your proposed vec<> would be quite radically > >>> different (and IMHO vec<> and auto_vec<> should be unified then to > >>> form your proposed new container). auto_vec<> at the moment simply > >>> maintains ownership like a smart pointer - which is _also_ not copyable. > >> > >> Yes, as we discussed in the review below, vec is not a good model > >> because (as you note again above) it's constrained by its legacy > >> uses. The best I think we can do for it is to make it safer to > >> use. > >> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html > > > > Which is what Trevors patches do by simply disallowing things > > that do not work at the moment. > > > >> (Smart pointers don't rule out copying. A std::unique_ptr does > >> and std::shared_ptr doesn't. But vec and especially auto_vec > >> are designed to be containers, not "unique pointers" so any > >> relationship there is purely superficial and a distraction.) > >> > >> That auto_vec and vec share a name and an is-a relationship is > >> incidental, an implementation detail leaked into the API. A better > >> name than vector is hard to come up with, but the public inheritance > >> is a design flaw, a bug waiting to be introduced due to the conversion > >> and the assumptions the base vec makes about POD-ness and shallow > >> copying. Hindsight is always 20/20 but past mistakes should not > >> dictate the design of a general purpose vector-like container in > >> GCC. > > > > That auto_vec<> "decays" to vec<> was
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On 6/18/21 4:38 AM, Richard Biener wrote: On Thu, Jun 17, 2021 at 4:43 PM Martin Sebor wrote: On 6/17/21 12:03 AM, Richard Biener wrote: On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor wrote: On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote: Richard Biener via Gcc-patches writes: On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders wrote: This makes it clear the caller owns the vector, and ensures it is cleaned up. Signed-off-by: Trevor Saunders bootstrapped and regtested on x86_64-linux-gnu, ok? OK. Btw, are "standard API" returns places we can use 'auto'? That would avoid excessive indent for - dom_bbs = get_dominated_by_region (CDI_DOMINATORS, -bbs.address (), -bbs.length ()); + auto_vec dom_bbs = get_dominated_by_region (CDI_DOMINATORS, + bbs.address (), + bbs.length ()); and just uses auto dom_bbs = get_dominated_by_region (... Not asking you to do this, just a question for the audience. Personally I think this would be surprising for something that doesn't have copy semantics. (Not that I'm trying to reopen that debate here :-) FWIW, I agree not having copy semantics is probably the most practical way forward for now.) But you did open the door for me to reiterate my strong disagreement with that. The best C++ practice going back to the early 1990's is to make types safely copyable and assignable. It is the default for all types, in both C++ and C, and so natural and expected. Preventing copying is appropriate in special and rare circumstances (e.g, a mutex may not be copyable, or a file or iostream object may not be because they represent a unique physical resource.) In the absence of such special circumstances preventing copying is unexpected, and in the case of an essential building block such as a container, makes the type difficult to use. The only argument for disabling copying that has been given is that it could be surprising(*). But because all types are copyable by default the "surprise" is usually when one can't be. I think Richi's "surprising" has to do with the fact that it lets one inadvertently copy a large amount of data, thus leading to an inefficiency. But by analogy, there are infinitely many ways to end up with inefficient code (e.g., deep recursion, or heap allocation in a loop), and they are not a reason to ban the coding constructs that might lead to it. IIUC, Jason's comment about surprising effects was about implicit conversion from auto_vec to vec. I share that concern, and agree that it should be addressed by preventing the conversion (as Jason suggested). But fact is that how vec<> and auto_vec<> are used today in GCC do not favor that. In fact your proposed vec<> would be quite radically different (and IMHO vec<> and auto_vec<> should be unified then to form your proposed new container). auto_vec<> at the moment simply maintains ownership like a smart pointer - which is _also_ not copyable. Yes, as we discussed in the review below, vec is not a good model because (as you note again above) it's constrained by its legacy uses. The best I think we can do for it is to make it safer to use. https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html Which is what Trevors patches do by simply disallowing things that do not work at the moment. (Smart pointers don't rule out copying. A std::unique_ptr does and std::shared_ptr doesn't. But vec and especially auto_vec are designed to be containers, not "unique pointers" so any relationship there is purely superficial and a distraction.) That auto_vec and vec share a name and an is-a relationship is incidental, an implementation detail leaked into the API. A better name than vector is hard to come up with, but the public inheritance is a design flaw, a bug waiting to be introduced due to the conversion and the assumptions the base vec makes about POD-ness and shallow copying. Hindsight is always 20/20 but past mistakes should not dictate the design of a general purpose vector-like container in GCC. That auto_vec<> "decays" to vec<> was on purpose design. By-value passing of vec<> is also on purpose to avoid an extra pointer indirection on each access. I think you may have misunderstood what I mean by is-a relationship. It's fine to convert an auto_vec to another interface. The danger is in allowing that to happen implicitly because that tends to let it happen even when it's not intended. The usual way to avoid that risk is to provide a conversion function, like auto_vec::to_vec(). This is also why standard classes like std::vector or std::string don't allow such implicit conversions and instead provide member functions (see for example Stroustrup: The C++ Programming Language). So a safer auto_vec class would not be publicly derived from vec but instead use the has-a desi
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Fri, 18 Jun 2021 at 12:03, Jonathan Wakely wrote: > > On Fri, 18 Jun 2021 at 11:54, Jakub Jelinek wrote: > > > > On Fri, Jun 18, 2021 at 12:38:09PM +0200, Richard Biener via Gcc-patches > > wrote: > > > > Yes, as we discussed in the review below, vec is not a good model > > > > because (as you note again above) it's constrained by its legacy > > > > uses. The best I think we can do for it is to make it safer to > > > > use. > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html > > > > > > Which is what Trevors patches do by simply disallowing things > > > that do not work at the moment. > > > > I only see > > // You probably don't want to copy a vector, so these are deleted to > > prevent > > // unintentional use. If you really need a copy of the vectors contents > > you > > // can use copy (). > > auto_vec(const auto_vec &) = delete; > > auto_vec &operator= (const auto_vec &) = delete; > > on the > > template > > class auto_vec : public vec > > specialization, but not on the > > template > > class auto_vec : public vec > > template itself. Shouldn't that one have also the deleted > > copy ctor/assignment operator and in addition to that maybe deleted > > move ctor/move assignment operator? > > That might have some value as documentation for people reading the > code, but it's not necessary. If vec has a deleted copy ctor and copy > assignment then it has no implicitly-defined move ctor and move > assignment. And the same goes for anything deriving from vec. Oh sorry, I misread the first snippet. So yes, it should probably be on both specializations. But deleting the moves is not necessary.
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Fri, 18 Jun 2021 at 11:54, Jakub Jelinek wrote: > > On Fri, Jun 18, 2021 at 12:38:09PM +0200, Richard Biener via Gcc-patches > wrote: > > > Yes, as we discussed in the review below, vec is not a good model > > > because (as you note again above) it's constrained by its legacy > > > uses. The best I think we can do for it is to make it safer to > > > use. > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html > > > > Which is what Trevors patches do by simply disallowing things > > that do not work at the moment. > > I only see > // You probably don't want to copy a vector, so these are deleted to prevent > // unintentional use. If you really need a copy of the vectors contents you > // can use copy (). > auto_vec(const auto_vec &) = delete; > auto_vec &operator= (const auto_vec &) = delete; > on the > template > class auto_vec : public vec > specialization, but not on the > template > class auto_vec : public vec > template itself. Shouldn't that one have also the deleted > copy ctor/assignment operator and in addition to that maybe deleted > move ctor/move assignment operator? That might have some value as documentation for people reading the code, but it's not necessary. If vec has a deleted copy ctor and copy assignment then it has no implicitly-defined move ctor and move assignment. And the same goes for anything deriving from vec.
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Fri, Jun 18, 2021 at 12:38:09PM +0200, Richard Biener via Gcc-patches wrote: > > Yes, as we discussed in the review below, vec is not a good model > > because (as you note again above) it's constrained by its legacy > > uses. The best I think we can do for it is to make it safer to > > use. > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html > > Which is what Trevors patches do by simply disallowing things > that do not work at the moment. I only see // You probably don't want to copy a vector, so these are deleted to prevent // unintentional use. If you really need a copy of the vectors contents you // can use copy (). auto_vec(const auto_vec &) = delete; auto_vec &operator= (const auto_vec &) = delete; on the template class auto_vec : public vec specialization, but not on the template class auto_vec : public vec template itself. Shouldn't that one have also the deleted copy ctor/assignment operator and in addition to that maybe deleted move ctor/move assignment operator? Jakub
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Thu, Jun 17, 2021 at 4:43 PM Martin Sebor wrote: > > On 6/17/21 12:03 AM, Richard Biener wrote: > > On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor wrote: > >> > >> On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote: > >>> Richard Biener via Gcc-patches writes: > On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders > wrote: > > > > This makes it clear the caller owns the vector, and ensures it is > > cleaned up. > > > > Signed-off-by: Trevor Saunders > > > > bootstrapped and regtested on x86_64-linux-gnu, ok? > > OK. > > Btw, are "standard API" returns places we can use 'auto'? That would > avoid > excessive indent for > > - dom_bbs = get_dominated_by_region (CDI_DOMINATORS, > -bbs.address (), > -bbs.length ()); > + auto_vec dom_bbs = get_dominated_by_region > (CDI_DOMINATORS, > + bbs.address > (), > + bbs.length > ()); > > and just uses > > auto dom_bbs = get_dominated_by_region (... > > Not asking you to do this, just a question for the audience. > >>> > >>> Personally I think this would be surprising for something that doesn't > >>> have copy semantics. (Not that I'm trying to reopen that debate here :-) > >>> FWIW, I agree not having copy semantics is probably the most practical > >>> way forward for now.) > >> > >> But you did open the door for me to reiterate my strong disagreement > >> with that. The best C++ practice going back to the early 1990's is > >> to make types safely copyable and assignable. It is the default for > >> all types, in both C++ and C, and so natural and expected. > >> > >> Preventing copying is appropriate in special and rare circumstances > >> (e.g, a mutex may not be copyable, or a file or iostream object may > >> not be because they represent a unique physical resource.) > >> > >> In the absence of such special circumstances preventing copying is > >> unexpected, and in the case of an essential building block such as > >> a container, makes the type difficult to use. > >> > >> The only argument for disabling copying that has been given is > >> that it could be surprising(*). But because all types are copyable > >> by default the "surprise" is usually when one can't be. > >> > >> I think Richi's "surprising" has to do with the fact that it lets > >> one inadvertently copy a large amount of data, thus leading to > >> an inefficiency. But by analogy, there are infinitely many ways > >> to end up with inefficient code (e.g., deep recursion, or heap > >> allocation in a loop), and they are not a reason to ban the coding > >> constructs that might lead to it. > >> > >> IIUC, Jason's comment about surprising effects was about implicit > >> conversion from auto_vec to vec. I share that concern, and agree > >> that it should be addressed by preventing the conversion (as Jason > >> suggested). > > > > But fact is that how vec<> and auto_vec<> are used today in GCC > > do not favor that. In fact your proposed vec<> would be quite radically > > different (and IMHO vec<> and auto_vec<> should be unified then to > > form your proposed new container). auto_vec<> at the moment simply > > maintains ownership like a smart pointer - which is _also_ not copyable. > > Yes, as we discussed in the review below, vec is not a good model > because (as you note again above) it's constrained by its legacy > uses. The best I think we can do for it is to make it safer to > use. > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html Which is what Trevors patches do by simply disallowing things that do not work at the moment. > (Smart pointers don't rule out copying. A std::unique_ptr does > and std::shared_ptr doesn't. But vec and especially auto_vec > are designed to be containers, not "unique pointers" so any > relationship there is purely superficial and a distraction.) > > That auto_vec and vec share a name and an is-a relationship is > incidental, an implementation detail leaked into the API. A better > name than vector is hard to come up with, but the public inheritance > is a design flaw, a bug waiting to be introduced due to the conversion > and the assumptions the base vec makes about POD-ness and shallow > copying. Hindsight is always 20/20 but past mistakes should not > dictate the design of a general purpose vector-like container in > GCC. That auto_vec<> "decays" to vec<> was on purpose design. By-value passing of vec<> is also on purpose to avoid an extra pointer indirection on each access. > I fully support fixing or at least mitigating the problems with > the vec base class (unsafe copying, pass-by-value etc.). As I > mentioned, I already started working on this cleanup. I also > have no objection to introduc
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On 6/17/21 12:03 AM, Richard Biener wrote: On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor wrote: On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote: Richard Biener via Gcc-patches writes: On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders wrote: This makes it clear the caller owns the vector, and ensures it is cleaned up. Signed-off-by: Trevor Saunders bootstrapped and regtested on x86_64-linux-gnu, ok? OK. Btw, are "standard API" returns places we can use 'auto'? That would avoid excessive indent for - dom_bbs = get_dominated_by_region (CDI_DOMINATORS, -bbs.address (), -bbs.length ()); + auto_vec dom_bbs = get_dominated_by_region (CDI_DOMINATORS, + bbs.address (), + bbs.length ()); and just uses auto dom_bbs = get_dominated_by_region (... Not asking you to do this, just a question for the audience. Personally I think this would be surprising for something that doesn't have copy semantics. (Not that I'm trying to reopen that debate here :-) FWIW, I agree not having copy semantics is probably the most practical way forward for now.) But you did open the door for me to reiterate my strong disagreement with that. The best C++ practice going back to the early 1990's is to make types safely copyable and assignable. It is the default for all types, in both C++ and C, and so natural and expected. Preventing copying is appropriate in special and rare circumstances (e.g, a mutex may not be copyable, or a file or iostream object may not be because they represent a unique physical resource.) In the absence of such special circumstances preventing copying is unexpected, and in the case of an essential building block such as a container, makes the type difficult to use. The only argument for disabling copying that has been given is that it could be surprising(*). But because all types are copyable by default the "surprise" is usually when one can't be. I think Richi's "surprising" has to do with the fact that it lets one inadvertently copy a large amount of data, thus leading to an inefficiency. But by analogy, there are infinitely many ways to end up with inefficient code (e.g., deep recursion, or heap allocation in a loop), and they are not a reason to ban the coding constructs that might lead to it. IIUC, Jason's comment about surprising effects was about implicit conversion from auto_vec to vec. I share that concern, and agree that it should be addressed by preventing the conversion (as Jason suggested). But fact is that how vec<> and auto_vec<> are used today in GCC do not favor that. In fact your proposed vec<> would be quite radically different (and IMHO vec<> and auto_vec<> should be unified then to form your proposed new container). auto_vec<> at the moment simply maintains ownership like a smart pointer - which is _also_ not copyable. Yes, as we discussed in the review below, vec is not a good model because (as you note again above) it's constrained by its legacy uses. The best I think we can do for it is to make it safer to use. https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html (Smart pointers don't rule out copying. A std::unique_ptr does and std::shared_ptr doesn't. But vec and especially auto_vec are designed to be containers, not "unique pointers" so any relationship there is purely superficial and a distraction.) That auto_vec and vec share a name and an is-a relationship is incidental, an implementation detail leaked into the API. A better name than vector is hard to come up with, but the public inheritance is a design flaw, a bug waiting to be introduced due to the conversion and the assumptions the base vec makes about POD-ness and shallow copying. Hindsight is always 20/20 but past mistakes should not dictate the design of a general purpose vector-like container in GCC. I fully support fixing or at least mitigating the problems with the vec base class (unsafe copying, pass-by-value etc.). As I mentioned, I already started working on this cleanup. I also have no objection to introducing a non-copyable form of a vector template (I offered one in my patch), or even to making auto_vec non-copyable provided a copyable and assignable one is introduced at the same time, under some other name. Having said that, and although I don't mind the cleanup being taken off my plate, I would have expected the courtesy of at least a heads up first. I do find it disrespectful for someone else involved in the review of my work to at the same time submit a patch of their own that goes in the opposite direction, and for you to unilaterally approve it while the other review hasn't concluded yet. Martin Richard. Martin Thanks, Richard Thanks, Richard. gcc/ChangeLog: * dominance.c (get_dominated_by_region): Return auto_vec. * domin
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Thu, Jun 17, 2021 at 08:03:53AM +0200, Richard Biener wrote: > On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor wrote: > > > > On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote: > > > Richard Biener via Gcc-patches writes: > > >> On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders > > >> wrote: > > >>> > > >>> This makes it clear the caller owns the vector, and ensures it is > > >>> cleaned up. > > >>> > > >>> Signed-off-by: Trevor Saunders > > >>> > > >>> bootstrapped and regtested on x86_64-linux-gnu, ok? > > >> > > >> OK. > > >> > > >> Btw, are "standard API" returns places we can use 'auto'? That would > > >> avoid > > >> excessive indent for > > >> > > >> - dom_bbs = get_dominated_by_region (CDI_DOMINATORS, > > >> -bbs.address (), > > >> -bbs.length ()); > > >> + auto_vec dom_bbs = get_dominated_by_region > > >> (CDI_DOMINATORS, > > >> + bbs.address > > >> (), > > >> + bbs.length > > >> ()); > > >> > > >> and just uses > > >> > > >>auto dom_bbs = get_dominated_by_region (... > > >> > > >> Not asking you to do this, just a question for the audience. > > > > > > Personally I think this would be surprising for something that doesn't > > > have copy semantics. (Not that I'm trying to reopen that debate here :-) > > > FWIW, I agree not having copy semantics is probably the most practical > > > way forward for now.) > > > > But you did open the door for me to reiterate my strong disagreement > > with that. The best C++ practice going back to the early 1990's is > > to make types safely copyable and assignable. It is the default for > > all types, in both C++ and C, and so natural and expected. For C its certainly true that any struct can be coppied with memcpy or plain assignment, but that doesn't mean it is expected to work, just look at vec pre c++, there was a copy macro to copy one. I don't believe there is a function to copy a hashtab hashtable, presumably because it was never needed, but here too we see a C datastructure that can simply be memcpy'd. Its rather narrow to only look at how C and C++ handle things, so lets look at some other languages that have learned from the previous 30 years. IN no particular order first looking at python https://www.afternerd.com/blog/python-copy-list/ seems like a reasonable summary, and we see assignment is just copying a reference to a list and you call .copy() to dupplicate the list. I believe java is essentially the same as python perhaps with slightly different names for methods. I'm not an expert but https://flaviocopes.com/go-copying-structs/ suggests go is the same as C except a little better since it has a GC so you don't have to refcount yourself. As for rust I may have linked it already, but sorry I will again, because I believe it makes a lot of good points, https://www.oreilly.com/library/view/programming-rust/9781491927274/ch04.html assignment moves variables, and you call a function to make a deeper copy. Its also worth noting the point that rust is enforcing good C++ practice, one of which I'd say is that you move or lend values whenever possible, rather than making coppies, certainly when copy constructors where the only thing available making them safe was the only option, but fortunately today's C++ is very different from the c++ of the 90's, and we shouldn't keep the same ideas of what's the best practice either. > > Preventing copying is appropriate in special and rare circumstances > > (e.g, a mutex may not be copyable, or a file or iostream object may > > not be because they represent a unique physical resource.) Well, copying a mutex can have definable semantics, but I doubt anyone actually wants copyable mutexs. As for files and streams its easy to copy files, and streams may be easy depending, and as building blocks one can certainly argue by your logic they should be copyable. Or we can take Richi's example, I would think by your logic an important building block like unique_ptr should support copying by making a copy of the referenced object, that would preserve the 1:1 relationship between unique_ptr objects and the objects they own. Further it would allow me to easily copy vector> or unordered_map>. I suspect you would not support making unique_ptr copyable, so I think the line of what should be copy constructable is less clear than your making it out to be here, certainly that's a rather extreme case, but I think under the framework you propose copyable unique_ptr is defensable. > > In the absence of such special circumstances preventing copying is > > unexpected, and in the case of an essential building block such as > > a container, makes the type difficult to use. > > > > The only argument for disabling copying that has been given is > > that it could be surprising(*). But because all types are copyable Sorry
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor wrote:
>
> On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote:
> > Richard Biener via Gcc-patches writes:
> >> On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders
> >> wrote:
> >>>
> >>> This makes it clear the caller owns the vector, and ensures it is cleaned
> >>> up.
> >>>
> >>> Signed-off-by: Trevor Saunders
> >>>
> >>> bootstrapped and regtested on x86_64-linux-gnu, ok?
> >>
> >> OK.
> >>
> >> Btw, are "standard API" returns places we can use 'auto'? That would avoid
> >> excessive indent for
> >>
> >> - dom_bbs = get_dominated_by_region (CDI_DOMINATORS,
> >> -bbs.address (),
> >> -bbs.length ());
> >> + auto_vec dom_bbs = get_dominated_by_region (CDI_DOMINATORS,
> >> + bbs.address (),
> >> + bbs.length ());
> >>
> >> and just uses
> >>
> >>auto dom_bbs = get_dominated_by_region (...
> >>
> >> Not asking you to do this, just a question for the audience.
> >
> > Personally I think this would be surprising for something that doesn't
> > have copy semantics. (Not that I'm trying to reopen that debate here :-)
> > FWIW, I agree not having copy semantics is probably the most practical
> > way forward for now.)
>
> But you did open the door for me to reiterate my strong disagreement
> with that. The best C++ practice going back to the early 1990's is
> to make types safely copyable and assignable. It is the default for
> all types, in both C++ and C, and so natural and expected.
>
> Preventing copying is appropriate in special and rare circumstances
> (e.g, a mutex may not be copyable, or a file or iostream object may
> not be because they represent a unique physical resource.)
>
> In the absence of such special circumstances preventing copying is
> unexpected, and in the case of an essential building block such as
> a container, makes the type difficult to use.
>
> The only argument for disabling copying that has been given is
> that it could be surprising(*). But because all types are copyable
> by default the "surprise" is usually when one can't be.
>
> I think Richi's "surprising" has to do with the fact that it lets
> one inadvertently copy a large amount of data, thus leading to
> an inefficiency. But by analogy, there are infinitely many ways
> to end up with inefficient code (e.g., deep recursion, or heap
> allocation in a loop), and they are not a reason to ban the coding
> constructs that might lead to it.
>
> IIUC, Jason's comment about surprising effects was about implicit
> conversion from auto_vec to vec. I share that concern, and agree
> that it should be addressed by preventing the conversion (as Jason
> suggested).
But fact is that how vec<> and auto_vec<> are used today in GCC
do not favor that. In fact your proposed vec<> would be quite radically
different (and IMHO vec<> and auto_vec<> should be unified then to
form your proposed new container). auto_vec<> at the moment simply
maintains ownership like a smart pointer - which is _also_ not copyable.
Richard.
> Martin
>
> >
> > Thanks,
> > Richard
> >
> >> Thanks,
> >> Richard.
> >>
> >>> gcc/ChangeLog:
> >>>
> >>> * dominance.c (get_dominated_by_region): Return
> >>> auto_vec.
> >>> * dominance.h (get_dominated_by_region): Likewise.
> >>> * tree-cfg.c (gimple_duplicate_sese_region): Adjust.
> >>> (gimple_duplicate_sese_tail): Likewise.
> >>> (move_sese_region_to_fn): Likewise.
> >>> ---
> >>> gcc/dominance.c | 4 ++--
> >>> gcc/dominance.h | 2 +-
> >>> gcc/tree-cfg.c | 18 +++---
> >>> 3 files changed, 10 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/gcc/dominance.c b/gcc/dominance.c
> >>> index 0e464cb7282..4943102ff1d 100644
> >>> --- a/gcc/dominance.c
> >>> +++ b/gcc/dominance.c
> >>> @@ -906,13 +906,13 @@ get_dominated_by (enum cdi_direction dir,
> >>> basic_block bb)
> >>> direction DIR) by some block between N_REGION ones stored in REGION,
> >>> except for blocks in the REGION itself. */
> >>>
> >>> -vec
> >>> +auto_vec
> >>> get_dominated_by_region (enum cdi_direction dir, basic_block *region,
> >>> unsigned n_region)
> >>> {
> >>> unsigned i;
> >>> basic_block dom;
> >>> - vec doms = vNULL;
> >>> + auto_vec doms;
> >>>
> >>> for (i = 0; i < n_region; i++)
> >>> region[i]->flags |= BB_DUPLICATED;
> >>> diff --git a/gcc/dominance.h b/gcc/dominance.h
> >>> index 515a369aacf..c74ad297c6a 100644
> >>> --- a/gcc/dominance.h
> >>> +++ b/gcc/dominance.h
> >>> @@ -47,7 +47,7 @@ extern basic_block get_immediate_dominator (enum
> >>> cdi_direction, basic_block);
> >>> extern void set_immediate_dominator (enum cdi_direction, basic_block,
> >>> basic_block);
> >>> extern auto_vec get_dominated_by (enum cdi_dir
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote:
Richard Biener via Gcc-patches writes:
On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders wrote:
This makes it clear the caller owns the vector, and ensures it is cleaned up.
Signed-off-by: Trevor Saunders
bootstrapped and regtested on x86_64-linux-gnu, ok?
OK.
Btw, are "standard API" returns places we can use 'auto'? That would avoid
excessive indent for
- dom_bbs = get_dominated_by_region (CDI_DOMINATORS,
-bbs.address (),
-bbs.length ());
+ auto_vec dom_bbs = get_dominated_by_region (CDI_DOMINATORS,
+ bbs.address (),
+ bbs.length ());
and just uses
auto dom_bbs = get_dominated_by_region (...
Not asking you to do this, just a question for the audience.
Personally I think this would be surprising for something that doesn't
have copy semantics. (Not that I'm trying to reopen that debate here :-)
FWIW, I agree not having copy semantics is probably the most practical
way forward for now.)
But you did open the door for me to reiterate my strong disagreement
with that. The best C++ practice going back to the early 1990's is
to make types safely copyable and assignable. It is the default for
all types, in both C++ and C, and so natural and expected.
Preventing copying is appropriate in special and rare circumstances
(e.g, a mutex may not be copyable, or a file or iostream object may
not be because they represent a unique physical resource.)
In the absence of such special circumstances preventing copying is
unexpected, and in the case of an essential building block such as
a container, makes the type difficult to use.
The only argument for disabling copying that has been given is
that it could be surprising(*). But because all types are copyable
by default the "surprise" is usually when one can't be.
I think Richi's "surprising" has to do with the fact that it lets
one inadvertently copy a large amount of data, thus leading to
an inefficiency. But by analogy, there are infinitely many ways
to end up with inefficient code (e.g., deep recursion, or heap
allocation in a loop), and they are not a reason to ban the coding
constructs that might lead to it.
IIUC, Jason's comment about surprising effects was about implicit
conversion from auto_vec to vec. I share that concern, and agree
that it should be addressed by preventing the conversion (as Jason
suggested).
Martin
Thanks,
Richard
Thanks,
Richard.
gcc/ChangeLog:
* dominance.c (get_dominated_by_region): Return auto_vec.
* dominance.h (get_dominated_by_region): Likewise.
* tree-cfg.c (gimple_duplicate_sese_region): Adjust.
(gimple_duplicate_sese_tail): Likewise.
(move_sese_region_to_fn): Likewise.
---
gcc/dominance.c | 4 ++--
gcc/dominance.h | 2 +-
gcc/tree-cfg.c | 18 +++---
3 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/gcc/dominance.c b/gcc/dominance.c
index 0e464cb7282..4943102ff1d 100644
--- a/gcc/dominance.c
+++ b/gcc/dominance.c
@@ -906,13 +906,13 @@ get_dominated_by (enum cdi_direction dir, basic_block bb)
direction DIR) by some block between N_REGION ones stored in REGION,
except for blocks in the REGION itself. */
-vec
+auto_vec
get_dominated_by_region (enum cdi_direction dir, basic_block *region,
unsigned n_region)
{
unsigned i;
basic_block dom;
- vec doms = vNULL;
+ auto_vec doms;
for (i = 0; i < n_region; i++)
region[i]->flags |= BB_DUPLICATED;
diff --git a/gcc/dominance.h b/gcc/dominance.h
index 515a369aacf..c74ad297c6a 100644
--- a/gcc/dominance.h
+++ b/gcc/dominance.h
@@ -47,7 +47,7 @@ extern basic_block get_immediate_dominator (enum
cdi_direction, basic_block);
extern void set_immediate_dominator (enum cdi_direction, basic_block,
basic_block);
extern auto_vec get_dominated_by (enum cdi_direction,
basic_block);
-extern vec get_dominated_by_region (enum cdi_direction,
+extern auto_vec get_dominated_by_region (enum cdi_direction,
basic_block *,
unsigned);
extern vec get_dominated_to_depth (enum cdi_direction,
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 6bdd1a561fd..c9403deed19 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -6495,7 +6495,6 @@ gimple_duplicate_sese_region (edge entry, edge exit,
bool free_region_copy = false, copying_header = false;
class loop *loop = entry->dest->loop_father;
edge exit_copy;
- vec doms = vNULL;
edge redirected;
profile_count total_count = profile_count::uninitialized ();
profile_count entry_count = profile_count::uninitialized ();
@@ -6549,9 +6548,9 @@ gimple_duplicate_sese_region (edge entry, e
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
Richard Biener via Gcc-patches writes:
> On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders wrote:
>>
>> This makes it clear the caller owns the vector, and ensures it is cleaned up.
>>
>> Signed-off-by: Trevor Saunders
>>
>> bootstrapped and regtested on x86_64-linux-gnu, ok?
>
> OK.
>
> Btw, are "standard API" returns places we can use 'auto'? That would avoid
> excessive indent for
>
> - dom_bbs = get_dominated_by_region (CDI_DOMINATORS,
> -bbs.address (),
> -bbs.length ());
> + auto_vec dom_bbs = get_dominated_by_region (CDI_DOMINATORS,
> + bbs.address (),
> + bbs.length ());
>
> and just uses
>
> auto dom_bbs = get_dominated_by_region (...
>
> Not asking you to do this, just a question for the audience.
Personally I think this would be surprising for something that doesn't
have copy semantics. (Not that I'm trying to reopen that debate here :-)
FWIW, I agree not having copy semantics is probably the most practical
way forward for now.)
Thanks,
Richard
> Thanks,
> Richard.
>
>> gcc/ChangeLog:
>>
>> * dominance.c (get_dominated_by_region): Return
>> auto_vec.
>> * dominance.h (get_dominated_by_region): Likewise.
>> * tree-cfg.c (gimple_duplicate_sese_region): Adjust.
>> (gimple_duplicate_sese_tail): Likewise.
>> (move_sese_region_to_fn): Likewise.
>> ---
>> gcc/dominance.c | 4 ++--
>> gcc/dominance.h | 2 +-
>> gcc/tree-cfg.c | 18 +++---
>> 3 files changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/gcc/dominance.c b/gcc/dominance.c
>> index 0e464cb7282..4943102ff1d 100644
>> --- a/gcc/dominance.c
>> +++ b/gcc/dominance.c
>> @@ -906,13 +906,13 @@ get_dominated_by (enum cdi_direction dir, basic_block
>> bb)
>> direction DIR) by some block between N_REGION ones stored in REGION,
>> except for blocks in the REGION itself. */
>>
>> -vec
>> +auto_vec
>> get_dominated_by_region (enum cdi_direction dir, basic_block *region,
>> unsigned n_region)
>> {
>>unsigned i;
>>basic_block dom;
>> - vec doms = vNULL;
>> + auto_vec doms;
>>
>>for (i = 0; i < n_region; i++)
>> region[i]->flags |= BB_DUPLICATED;
>> diff --git a/gcc/dominance.h b/gcc/dominance.h
>> index 515a369aacf..c74ad297c6a 100644
>> --- a/gcc/dominance.h
>> +++ b/gcc/dominance.h
>> @@ -47,7 +47,7 @@ extern basic_block get_immediate_dominator (enum
>> cdi_direction, basic_block);
>> extern void set_immediate_dominator (enum cdi_direction, basic_block,
>> basic_block);
>> extern auto_vec get_dominated_by (enum cdi_direction,
>> basic_block);
>> -extern vec get_dominated_by_region (enum cdi_direction,
>> +extern auto_vec get_dominated_by_region (enum cdi_direction,
>> basic_block *,
>> unsigned);
>> extern vec get_dominated_to_depth (enum cdi_direction,
>> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
>> index 6bdd1a561fd..c9403deed19 100644
>> --- a/gcc/tree-cfg.c
>> +++ b/gcc/tree-cfg.c
>> @@ -6495,7 +6495,6 @@ gimple_duplicate_sese_region (edge entry, edge exit,
>>bool free_region_copy = false, copying_header = false;
>>class loop *loop = entry->dest->loop_father;
>>edge exit_copy;
>> - vec doms = vNULL;
>>edge redirected;
>>profile_count total_count = profile_count::uninitialized ();
>>profile_count entry_count = profile_count::uninitialized ();
>> @@ -6549,9 +6548,9 @@ gimple_duplicate_sese_region (edge entry, edge exit,
>>
>>/* Record blocks outside the region that are dominated by something
>> inside. */
>> + auto_vec doms;
>>if (update_dominance)
>> {
>> - doms.create (0);
>>doms = get_dominated_by_region (CDI_DOMINATORS, region, n_region);
>> }
>>
>> @@ -6596,7 +6595,6 @@ gimple_duplicate_sese_region (edge entry, edge exit,
>>set_immediate_dominator (CDI_DOMINATORS, entry->dest, entry->src);
>>doms.safe_push (get_bb_original (entry->dest));
>>iterate_fix_dominators (CDI_DOMINATORS, doms, false);
>> - doms.release ();
>> }
>>
>>/* Add the other PHI node arguments. */
>> @@ -6662,7 +6660,6 @@ gimple_duplicate_sese_tail (edge entry, edge exit,
>>class loop *loop = exit->dest->loop_father;
>>class loop *orig_loop = entry->dest->loop_father;
>>basic_block switch_bb, entry_bb, nentry_bb;
>> - vec doms;
>>profile_count total_count = profile_count::uninitialized (),
>> exit_count = profile_count::uninitialized ();
>>edge exits[2], nexits[2], e;
>> @@ -6705,7 +6702,8 @@ gimple_duplicate_sese_tail (edge entry, edge exit,
>>
>>/* Record blocks outside the region that are dominated by something
>> inside. */
>> - doms = get_dominated_by_region
Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders wrote:
>
> This makes it clear the caller owns the vector, and ensures it is cleaned up.
>
> Signed-off-by: Trevor Saunders
>
> bootstrapped and regtested on x86_64-linux-gnu, ok?
OK.
Btw, are "standard API" returns places we can use 'auto'? That would avoid
excessive indent for
- dom_bbs = get_dominated_by_region (CDI_DOMINATORS,
-bbs.address (),
-bbs.length ());
+ auto_vec dom_bbs = get_dominated_by_region (CDI_DOMINATORS,
+ bbs.address (),
+ bbs.length ());
and just uses
auto dom_bbs = get_dominated_by_region (...
Not asking you to do this, just a question for the audience.
Thanks,
Richard.
> gcc/ChangeLog:
>
> * dominance.c (get_dominated_by_region): Return auto_vec.
> * dominance.h (get_dominated_by_region): Likewise.
> * tree-cfg.c (gimple_duplicate_sese_region): Adjust.
> (gimple_duplicate_sese_tail): Likewise.
> (move_sese_region_to_fn): Likewise.
> ---
> gcc/dominance.c | 4 ++--
> gcc/dominance.h | 2 +-
> gcc/tree-cfg.c | 18 +++---
> 3 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/gcc/dominance.c b/gcc/dominance.c
> index 0e464cb7282..4943102ff1d 100644
> --- a/gcc/dominance.c
> +++ b/gcc/dominance.c
> @@ -906,13 +906,13 @@ get_dominated_by (enum cdi_direction dir, basic_block
> bb)
> direction DIR) by some block between N_REGION ones stored in REGION,
> except for blocks in the REGION itself. */
>
> -vec
> +auto_vec
> get_dominated_by_region (enum cdi_direction dir, basic_block *region,
> unsigned n_region)
> {
>unsigned i;
>basic_block dom;
> - vec doms = vNULL;
> + auto_vec doms;
>
>for (i = 0; i < n_region; i++)
> region[i]->flags |= BB_DUPLICATED;
> diff --git a/gcc/dominance.h b/gcc/dominance.h
> index 515a369aacf..c74ad297c6a 100644
> --- a/gcc/dominance.h
> +++ b/gcc/dominance.h
> @@ -47,7 +47,7 @@ extern basic_block get_immediate_dominator (enum
> cdi_direction, basic_block);
> extern void set_immediate_dominator (enum cdi_direction, basic_block,
> basic_block);
> extern auto_vec get_dominated_by (enum cdi_direction,
> basic_block);
> -extern vec get_dominated_by_region (enum cdi_direction,
> +extern auto_vec get_dominated_by_region (enum cdi_direction,
> basic_block *,
> unsigned);
> extern vec get_dominated_to_depth (enum cdi_direction,
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index 6bdd1a561fd..c9403deed19 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -6495,7 +6495,6 @@ gimple_duplicate_sese_region (edge entry, edge exit,
>bool free_region_copy = false, copying_header = false;
>class loop *loop = entry->dest->loop_father;
>edge exit_copy;
> - vec doms = vNULL;
>edge redirected;
>profile_count total_count = profile_count::uninitialized ();
>profile_count entry_count = profile_count::uninitialized ();
> @@ -6549,9 +6548,9 @@ gimple_duplicate_sese_region (edge entry, edge exit,
>
>/* Record blocks outside the region that are dominated by something
> inside. */
> + auto_vec doms;
>if (update_dominance)
> {
> - doms.create (0);
>doms = get_dominated_by_region (CDI_DOMINATORS, region, n_region);
> }
>
> @@ -6596,7 +6595,6 @@ gimple_duplicate_sese_region (edge entry, edge exit,
>set_immediate_dominator (CDI_DOMINATORS, entry->dest, entry->src);
>doms.safe_push (get_bb_original (entry->dest));
>iterate_fix_dominators (CDI_DOMINATORS, doms, false);
> - doms.release ();
> }
>
>/* Add the other PHI node arguments. */
> @@ -6662,7 +6660,6 @@ gimple_duplicate_sese_tail (edge entry, edge exit,
>class loop *loop = exit->dest->loop_father;
>class loop *orig_loop = entry->dest->loop_father;
>basic_block switch_bb, entry_bb, nentry_bb;
> - vec doms;
>profile_count total_count = profile_count::uninitialized (),
> exit_count = profile_count::uninitialized ();
>edge exits[2], nexits[2], e;
> @@ -6705,7 +6702,8 @@ gimple_duplicate_sese_tail (edge entry, edge exit,
>
>/* Record blocks outside the region that are dominated by something
> inside. */
> - doms = get_dominated_by_region (CDI_DOMINATORS, region, n_region);
> + auto_vec doms = get_dominated_by_region (CDI_DOMINATORS,
> region,
> + n_region);
>
>total_count = exit->src->count;
>exit_count = exit->count ();
> @@ -6785,7 +6783,6 @@ gimple_duplicate_sese_tail (edge entry, edge exit,
>/* Anything that is outside of the region, but was dominated by something
> inside needs to update
