Re: [PATCH] c++: Implement -Wrange-loop-construct [PR94695]
On 9/28/20 11:34 AM, Marek Polacek wrote:
On Fri, Sep 25, 2020 at 04:31:16PM -0600, Martin Sebor wrote:
On 9/24/20 6:05 PM, Marek Polacek via Gcc-patches wrote:
This new warning can be used to prevent expensive copies inside range-based
for-loops, for instance:
struct S { char arr[128]; };
void fn () {
S arr[5];
for (const auto x : arr) { }
}
where auto deduces to S and then we copy the big S in every iteration.
Using "const auto " would not incur such a copy. With this patch the
compiler will warn:
q.C:4:19: warning: loop variable 'x' creates a copy from type 'const S'
[-Wrange-loop-construct]
4 | for (const auto x : arr) { }
| ^
q.C:4:19: note: use reference type 'const S&' to prevent copying
4 | for (const auto x : arr) { }
| ^
| &
As per Clang, this warning is suppressed for trivially copyable types
whose size does not exceed 64B. The tricky part of the patch was how
to figure out if using a reference would have prevented a copy. I've
used perform_implicit_conversion to perform the imaginary conversion.
Then if the conversion doesn't have any side-effects, I assume it does
not call any functions or create any TARGET_EXPRs, and is just a simple
assignment like this one:
const T = (const T &) <__for_begin>;
But it can also be a CALL_EXPR:
x = (const T &) Iterator::operator* (&__for_begin)
which is still fine -- we just use the return value and don't create
any copies.
This warning is enabled by -Wall. Further warnings of similar nature
should follow soon.
I've always thought a warning like this would be useful when passing
large objects to functions by value. Is adding one for these cases
what you mean by future warnings?
No, but perhaps we should add it. I don't know if we could still enable it by
-Wall. We'd have to handle guaranteed copy elision and also the case when we
pass classes by invisible reference. Unsure how much of the implementation
these warnings could share.
Do we have a request for the warning wrt passing chunky objects by value?
Not that I know of. It's just something I had in the back of my
mind.
As a user, I'd probably want to have the option of figuring out where I'm
copying large types, since it can be a performance issue.
For the range loop, I wonder if more could be done to elide the copy
and avoid the warning when it isn't really necessary. For instance,
for trivially copyable types like in your example, since x is const,
modifying it would be undefined, and so when we can prove that
the original object itself isn't modified (e.g., because it's
declared const, or because it can't be accessed in the loop),
there should be no need to make a copy on each iteration. Using
a reference to the original object should be sufficient. Does C++
rule out such an optimization?
Well, changing const auto x in
struct S { char arr[128]; S(); };
void
fn ()
{
S a[5];
for (const auto x : a)
decltype(x) k;
}
to const auto would break this code.
Sure, an optimization that changed code in a detectable way would
not be viable. But I wasn't thinking of actually changing the type
of the variable at this high level. What I meant is that it would
be nice to transform an example like this:
struct S { int i; char a[80]; };
const S a[] = { { 123, "abc" }, { 234, "bcd" }, { 345, "cde"} };
int f (const char *s)
{
for (auto x: a)
if (__builtin_strcmp (x.a, s) == 0)
return x.i;
return -1;
}
into this (when it's possible) and avoid issuing the warning:
int g (const char *s)
{
for (int i = 0; i != sizeof a / sizeof *a; ++i)
if (strcmp (a[i].a, s) == 0)
return a[i].i;
return -1;
}
About the name of the option: my first thought was that it was
about the construct known as the range loop, but after reading
your description I wonder if it might actually primarily be about
constructing expensive copies and the range loop is incidental.
It was a bit confusing to me too at first. It's about constructing expensive
copies in range-based for-loops. I don't think it's incidental that
it warns in loops only.
I'm not attached to the name but it's what Clang uses so we'll have to
follow.
(It's impossible to tell from the Clang manual because its way
of documenting warning options is to show examples of their text.)
Yes. I really like that we provide code snippets showing what a warning
is supposed to warn on in our manual. Let's keep it that way.
Then again, I see it's related to -Wrange-loop-analysis so that
suggests it is mainly about range loops, and that there may be
a whole series of warnings and options related to it. Can you
please shed some light on that? (E.g., what are some of
the "further warnings of similar nature" about?) I think it
might also be helpful to expand the documentation a bit to help
answer common questions (I came across the following
Re: [PATCH] c++: Implement -Wrange-loop-construct [PR94695]
On Fri, Sep 25, 2020 at 04:31:16PM -0600, Martin Sebor wrote:
> On 9/24/20 6:05 PM, Marek Polacek via Gcc-patches wrote:
> > This new warning can be used to prevent expensive copies inside range-based
> > for-loops, for instance:
> >
> >struct S { char arr[128]; };
> >void fn () {
> > S arr[5];
> > for (const auto x : arr) { }
> >}
> >
> > where auto deduces to S and then we copy the big S in every iteration.
> > Using "const auto " would not incur such a copy. With this patch the
> > compiler will warn:
> >
> > q.C:4:19: warning: loop variable 'x' creates a copy from type 'const S'
> > [-Wrange-loop-construct]
> > 4 | for (const auto x : arr) { }
> >| ^
> > q.C:4:19: note: use reference type 'const S&' to prevent copying
> > 4 | for (const auto x : arr) { }
> >| ^
> >| &
> >
> > As per Clang, this warning is suppressed for trivially copyable types
> > whose size does not exceed 64B. The tricky part of the patch was how
> > to figure out if using a reference would have prevented a copy. I've
> > used perform_implicit_conversion to perform the imaginary conversion.
> > Then if the conversion doesn't have any side-effects, I assume it does
> > not call any functions or create any TARGET_EXPRs, and is just a simple
> > assignment like this one:
> >
> >const T = (const T &) <__for_begin>;
> >
> > But it can also be a CALL_EXPR:
> >
> >x = (const T &) Iterator::operator* (&__for_begin)
> >
> > which is still fine -- we just use the return value and don't create
> > any copies.
> >
> > This warning is enabled by -Wall. Further warnings of similar nature
> > should follow soon.
>
> I've always thought a warning like this would be useful when passing
> large objects to functions by value. Is adding one for these cases
> what you mean by future warnings?
No, but perhaps we should add it. I don't know if we could still enable it by
-Wall. We'd have to handle guaranteed copy elision and also the case when we
pass classes by invisible reference. Unsure how much of the implementation
these warnings could share.
Do we have a request for the warning wrt passing chunky objects by value?
As a user, I'd probably want to have the option of figuring out where I'm
copying large types, since it can be a performance issue.
> For the range loop, I wonder if more could be done to elide the copy
> and avoid the warning when it isn't really necessary. For instance,
> for trivially copyable types like in your example, since x is const,
> modifying it would be undefined, and so when we can prove that
> the original object itself isn't modified (e.g., because it's
> declared const, or because it can't be accessed in the loop),
> there should be no need to make a copy on each iteration. Using
> a reference to the original object should be sufficient. Does C++
> rule out such an optimization?
Well, changing const auto x in
struct S { char arr[128]; S(); };
void
fn ()
{
S a[5];
for (const auto x : a)
decltype(x) k;
}
to const auto would break this code.
> About the name of the option: my first thought was that it was
> about the construct known as the range loop, but after reading
> your description I wonder if it might actually primarily be about
> constructing expensive copies and the range loop is incidental.
It was a bit confusing to me too at first. It's about constructing expensive
copies in range-based for-loops. I don't think it's incidental that
it warns in loops only.
I'm not attached to the name but it's what Clang uses so we'll have to
follow.
> (It's impossible to tell from the Clang manual because its way
> of documenting warning options is to show examples of their text.)
Yes. I really like that we provide code snippets showing what a warning
is supposed to warn on in our manual. Let's keep it that way.
> Then again, I see it's related to -Wrange-loop-analysis so that
> suggests it is mainly about range loops, and that there may be
> a whole series of warnings and options related to it. Can you
> please shed some light on that? (E.g., what are some of
> the "further warnings of similar nature" about?) I think it
> might also be helpful to expand the documentation a bit to help
> answer common questions (I came across the following post while
> looking it up: https://stackoverflow.com/questions/50066139).
I think right now it's like this (note the names and wording changed recently,
this is the latest version):
-> -Wfor-loop-analysis
|
-Wloop-analysis -| -> -Wrange-loop-bind-reference
| |
-> -Wrange-loop-analysis -|
|
-> -Wrange-loop-construct
* -Wloop-analysis and -Wrange-loop-analysis are umbrella flags.
Re: [PATCH] c++: Implement -Wrange-loop-construct [PR94695]
On 9/24/20 6:05 PM, Marek Polacek via Gcc-patches wrote:
This new warning can be used to prevent expensive copies inside range-based
for-loops, for instance:
struct S { char arr[128]; };
void fn () {
S arr[5];
for (const auto x : arr) { }
}
where auto deduces to S and then we copy the big S in every iteration.
Using "const auto " would not incur such a copy. With this patch the
compiler will warn:
q.C:4:19: warning: loop variable 'x' creates a copy from type 'const S'
[-Wrange-loop-construct]
4 | for (const auto x : arr) { }
| ^
q.C:4:19: note: use reference type 'const S&' to prevent copying
4 | for (const auto x : arr) { }
| ^
| &
As per Clang, this warning is suppressed for trivially copyable types
whose size does not exceed 64B. The tricky part of the patch was how
to figure out if using a reference would have prevented a copy. I've
used perform_implicit_conversion to perform the imaginary conversion.
Then if the conversion doesn't have any side-effects, I assume it does
not call any functions or create any TARGET_EXPRs, and is just a simple
assignment like this one:
const T = (const T &) <__for_begin>;
But it can also be a CALL_EXPR:
x = (const T &) Iterator::operator* (&__for_begin)
which is still fine -- we just use the return value and don't create
any copies.
This warning is enabled by -Wall. Further warnings of similar nature
should follow soon.
I've always thought a warning like this would be useful when passing
large objects to functions by value. Is adding one for these cases
what you mean by future warnings?
For the range loop, I wonder if more could be done to elide the copy
and avoid the warning when it isn't really necessary. For instance,
for trivially copyable types like in your example, since x is const,
modifying it would be undefined, and so when we can prove that
the original object itself isn't modified (e.g., because it's
declared const, or because it can't be accessed in the loop),
there should be no need to make a copy on each iteration. Using
a reference to the original object should be sufficient. Does C++
rule out such an optimization?
About the name of the option: my first thought was that it was
about the construct known as the range loop, but after reading
your description I wonder if it might actually primarily be about
constructing expensive copies and the range loop is incidental.
(It's impossible to tell from the Clang manual because its way
of documenting warning options is to show examples of their text.)
Then again, I see it's related to -Wrange-loop-analysis so that
suggests it is mainly about range loops, and that there may be
a whole series of warnings and options related to it. Can you
please shed some light on that? (E.g., what are some of
the "further warnings of similar nature" about?) I think it
might also be helpful to expand the documentation a bit to help
answer common questions (I came across the following post while
looking it up: https://stackoverflow.com/questions/50066139).
Martin
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
gcc/c-family/ChangeLog:
PR c++/94695
* c.opt (Wrange-loop-construct): New option.
gcc/cp/ChangeLog:
PR c++/94695
* parser.c (warn_for_range_copy): New function.
(cp_convert_range_for): Call it.
gcc/ChangeLog:
PR c++/94695
* doc/invoke.texi: Document -Wrange-loop-construct.
gcc/testsuite/ChangeLog:
PR c++/94695
* g++.dg/warn/Wrange-loop-construct.C: New test.
---
gcc/c-family/c.opt| 4 +
gcc/cp/parser.c | 77 ++-
gcc/doc/invoke.texi | 21 +-
.../g++.dg/warn/Wrange-loop-construct.C | 207 ++
4 files changed, 304 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Wrange-loop-construct.C
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 7761eefd203..bbf7da89658 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -800,6 +800,10 @@ Wpacked-not-aligned
C ObjC C++ ObjC++ Var(warn_packed_not_aligned) Warning LangEnabledBy(C ObjC
C++ ObjC++,Wall)
Warn when fields in a struct with the packed attribute are misaligned.
+Wrange-loop-construct
+C++ ObjC++ Var(warn_range_loop_construct) Warning LangEnabledBy(C++
ObjC++,Wall)
+Warn when a range-based for-loop is creating unnecessary copies.
+
Wredundant-tags
C++ ObjC++ Var(warn_redundant_tags) Warning
Warn when a class or enumerated type is referenced using a redundant
class-key.
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index fba3fcc0c4c..d233279ac62 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -12646,6 +12646,73 @@ do_range_for_auto_deduction (tree decl, tree
range_expr)
}
}
+/* Warns when the loop variable should be changed to a reference type to
+
Re: [PATCH] c++: Implement -Wrange-loop-construct [PR94695]
On 9/24/20 8:05 PM, Marek Polacek wrote:
This new warning can be used to prevent expensive copies inside range-based
for-loops, for instance:
struct S { char arr[128]; };
void fn () {
S arr[5];
for (const auto x : arr) { }
}
where auto deduces to S and then we copy the big S in every iteration.
Using "const auto " would not incur such a copy. With this patch the
compiler will warn:
q.C:4:19: warning: loop variable 'x' creates a copy from type 'const S'
[-Wrange-loop-construct]
4 | for (const auto x : arr) { }
| ^
q.C:4:19: note: use reference type 'const S&' to prevent copying
4 | for (const auto x : arr) { }
| ^
| &
As per Clang, this warning is suppressed for trivially copyable types
whose size does not exceed 64B. The tricky part of the patch was how
to figure out if using a reference would have prevented a copy. I've
used perform_implicit_conversion to perform the imaginary conversion.
Then if the conversion doesn't have any side-effects, I assume it does
not call any functions or create any TARGET_EXPRs, and is just a simple
assignment like this one:
const T = (const T &) <__for_begin>;
But it can also be a CALL_EXPR:
x = (const T &) Iterator::operator* (&__for_begin)
which is still fine -- we just use the return value and don't create
any copies.
Would conv_binds_ref_to_prvalue (implicit_conversion (...)) do what you
want?
This warning is enabled by -Wall. Further warnings of similar nature
should follow soon.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
gcc/c-family/ChangeLog:
PR c++/94695
* c.opt (Wrange-loop-construct): New option.
gcc/cp/ChangeLog:
PR c++/94695
* parser.c (warn_for_range_copy): New function.
(cp_convert_range_for): Call it.
gcc/ChangeLog:
PR c++/94695
* doc/invoke.texi: Document -Wrange-loop-construct.
gcc/testsuite/ChangeLog:
PR c++/94695
* g++.dg/warn/Wrange-loop-construct.C: New test.
---
gcc/c-family/c.opt| 4 +
gcc/cp/parser.c | 77 ++-
gcc/doc/invoke.texi | 21 +-
.../g++.dg/warn/Wrange-loop-construct.C | 207 ++
4 files changed, 304 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Wrange-loop-construct.C
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 7761eefd203..bbf7da89658 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -800,6 +800,10 @@ Wpacked-not-aligned
C ObjC C++ ObjC++ Var(warn_packed_not_aligned) Warning LangEnabledBy(C ObjC
C++ ObjC++,Wall)
Warn when fields in a struct with the packed attribute are misaligned.
+Wrange-loop-construct
+C++ ObjC++ Var(warn_range_loop_construct) Warning LangEnabledBy(C++
ObjC++,Wall)
+Warn when a range-based for-loop is creating unnecessary copies.
+
Wredundant-tags
C++ ObjC++ Var(warn_redundant_tags) Warning
Warn when a class or enumerated type is referenced using a redundant
class-key.
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index fba3fcc0c4c..d233279ac62 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -12646,6 +12646,73 @@ do_range_for_auto_deduction (tree decl, tree
range_expr)
}
}
+/* Warns when the loop variable should be changed to a reference type to
+ avoid unnecessary copying. I.e., from
+
+ for (const auto x : range)
+
+ where range returns a reference, to
+
+ for (const auto : range)
+
+ if this version doesn't make a copy. DECL is the RANGE_DECL; EXPR is the
+ *__for_begin expression.
+ This function is never called when processing_template_decl is on. */
+
+static void
+warn_for_range_copy (tree decl, tree expr)
+{
+ if (!warn_range_loop_construct
+ || decl == error_mark_node)
+return;
+
+ location_t loc = DECL_SOURCE_LOCATION (decl);
+ tree type = TREE_TYPE (decl);
+
+ if (from_macro_expansion_at (loc))
+return;
+
+ if (TYPE_REF_P (type))
+{
+ /* TODO: Implement reference warnings. */
+ return;
+}
+ else if (!CP_TYPE_CONST_P (type))
+return;
+
+ /* Since small trivially copyable types are cheap to copy, we suppress the
+ warning for them. 64B is a common size of a cache line. */
+ if (TREE_CODE (TYPE_SIZE_UNIT (type)) != INTEGER_CST
+ || (tree_to_uhwi (TYPE_SIZE_UNIT (type)) <= 64
+ && trivially_copyable_p (type)))
+return;
+
+ tree rtype = cp_build_reference_type (type, /*rval*/false);
+ /* See what it would take to convert the expr if we used a reference. */
+ expr = perform_implicit_conversion (rtype, expr, tf_none);
+ if (!TREE_SIDE_EFFECTS (expr))
+/* No calls/TARGET_EXPRs. */;
+ else
+{
+ /* If we could initialize the reference directly from the call, it
+wouldn't involve any copies. */
+ STRIP_NOPS (expr);
+ if (TREE_CODE (expr) != CALL_EXPR
[PATCH] c++: Implement -Wrange-loop-construct [PR94695]
This new warning can be used to prevent expensive copies inside range-based
for-loops, for instance:
struct S { char arr[128]; };
void fn () {
S arr[5];
for (const auto x : arr) { }
}
where auto deduces to S and then we copy the big S in every iteration.
Using "const auto " would not incur such a copy. With this patch the
compiler will warn:
q.C:4:19: warning: loop variable 'x' creates a copy from type 'const S'
[-Wrange-loop-construct]
4 | for (const auto x : arr) { }
| ^
q.C:4:19: note: use reference type 'const S&' to prevent copying
4 | for (const auto x : arr) { }
| ^
| &
As per Clang, this warning is suppressed for trivially copyable types
whose size does not exceed 64B. The tricky part of the patch was how
to figure out if using a reference would have prevented a copy. I've
used perform_implicit_conversion to perform the imaginary conversion.
Then if the conversion doesn't have any side-effects, I assume it does
not call any functions or create any TARGET_EXPRs, and is just a simple
assignment like this one:
const T = (const T &) <__for_begin>;
But it can also be a CALL_EXPR:
x = (const T &) Iterator::operator* (&__for_begin)
which is still fine -- we just use the return value and don't create
any copies.
This warning is enabled by -Wall. Further warnings of similar nature
should follow soon.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
gcc/c-family/ChangeLog:
PR c++/94695
* c.opt (Wrange-loop-construct): New option.
gcc/cp/ChangeLog:
PR c++/94695
* parser.c (warn_for_range_copy): New function.
(cp_convert_range_for): Call it.
gcc/ChangeLog:
PR c++/94695
* doc/invoke.texi: Document -Wrange-loop-construct.
gcc/testsuite/ChangeLog:
PR c++/94695
* g++.dg/warn/Wrange-loop-construct.C: New test.
---
gcc/c-family/c.opt| 4 +
gcc/cp/parser.c | 77 ++-
gcc/doc/invoke.texi | 21 +-
.../g++.dg/warn/Wrange-loop-construct.C | 207 ++
4 files changed, 304 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Wrange-loop-construct.C
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 7761eefd203..bbf7da89658 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -800,6 +800,10 @@ Wpacked-not-aligned
C ObjC C++ ObjC++ Var(warn_packed_not_aligned) Warning LangEnabledBy(C ObjC
C++ ObjC++,Wall)
Warn when fields in a struct with the packed attribute are misaligned.
+Wrange-loop-construct
+C++ ObjC++ Var(warn_range_loop_construct) Warning LangEnabledBy(C++
ObjC++,Wall)
+Warn when a range-based for-loop is creating unnecessary copies.
+
Wredundant-tags
C++ ObjC++ Var(warn_redundant_tags) Warning
Warn when a class or enumerated type is referenced using a redundant class-key.
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index fba3fcc0c4c..d233279ac62 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -12646,6 +12646,73 @@ do_range_for_auto_deduction (tree decl, tree
range_expr)
}
}
+/* Warns when the loop variable should be changed to a reference type to
+ avoid unnecessary copying. I.e., from
+
+ for (const auto x : range)
+
+ where range returns a reference, to
+
+ for (const auto : range)
+
+ if this version doesn't make a copy. DECL is the RANGE_DECL; EXPR is the
+ *__for_begin expression.
+ This function is never called when processing_template_decl is on. */
+
+static void
+warn_for_range_copy (tree decl, tree expr)
+{
+ if (!warn_range_loop_construct
+ || decl == error_mark_node)
+return;
+
+ location_t loc = DECL_SOURCE_LOCATION (decl);
+ tree type = TREE_TYPE (decl);
+
+ if (from_macro_expansion_at (loc))
+return;
+
+ if (TYPE_REF_P (type))
+{
+ /* TODO: Implement reference warnings. */
+ return;
+}
+ else if (!CP_TYPE_CONST_P (type))
+return;
+
+ /* Since small trivially copyable types are cheap to copy, we suppress the
+ warning for them. 64B is a common size of a cache line. */
+ if (TREE_CODE (TYPE_SIZE_UNIT (type)) != INTEGER_CST
+ || (tree_to_uhwi (TYPE_SIZE_UNIT (type)) <= 64
+ && trivially_copyable_p (type)))
+return;
+
+ tree rtype = cp_build_reference_type (type, /*rval*/false);
+ /* See what it would take to convert the expr if we used a reference. */
+ expr = perform_implicit_conversion (rtype, expr, tf_none);
+ if (!TREE_SIDE_EFFECTS (expr))
+/* No calls/TARGET_EXPRs. */;
+ else
+{
+ /* If we could initialize the reference directly from the call, it
+wouldn't involve any copies. */
+ STRIP_NOPS (expr);
+ if (TREE_CODE (expr) != CALL_EXPR
+ || !reference_related_p (non_reference (TREE_TYPE (expr)), type))
+ return;
+}
+
+ auto_diagnostic_group d;
+ if (warning_at
