void f(){ int i __attribute__((vector_size(2*sizeof(int)))) = { 2, 3 }; __builtin_shuffle (i, i); } g++-4.8: x.c:3:3: warning: statement has no effect [-Wunused-value] gcc-4.9: x.c:3:3: warning: value computed is not used [-Wunused-value] g++-4.9: nothing :-( We do warn for i+i, but not for (i+i) (again this is missing only in C++).
Confirmed.
For __builtin_shuffle, the issue is that we now call save_expr, which always sets TREE_SIDE_EFFECTS to 1. I don't know if it would make sense to introduce a maybe_save_expr that is equivalent to save_expr but does not set TREE_SIDE_EFFECTS if its argument doesn't have it. But in any case I think we still want to warn for an unused result in __builtin_shuffle(x,++m) so that's not the solution. In C we also have the side_effects_flag but we still warn in warn_if_unused_value (the default for unknown trees), whereas in C++ (near the end of convert_to_void in cvt.c) only some tcc_comparison, tcc_unary and tcc_binary can warn when they have side effects. It would be easy to add VEC_PERM_EXPR to the list and get a "value computed is not used", I just don't know if something more general is possible. For (i+i), the PLUS_EXPR ends up with nowarning_flag = 1 somehow.
You could decorate __builtin_shuffle with attribute "warn_unused_result". It is not the same, but it probably more precise. It would also warn for: __builtin_shuffle(x,++m), while -Wunused-value would not.
(In reply to Manuel López-Ibáñez from comment #3) > You could decorate __builtin_shuffle with attribute "warn_unused_result". It > is not the same, but it probably more precise. It would also warn for: > __builtin_shuffle(x,++m), while -Wunused-value would not. Would it? The front-ends replace __builtin_shuffle with VEC_PERM_EXPR quite early. I am not even sure we have a function type to which we could attach the attribute.
> For __builtin_shuffle, the issue is that we now call save_expr, which always > sets TREE_SIDE_EFFECTS to 1. I don't know if it would make sense to > introduce a maybe_save_expr that is equivalent to save_expr but does not set > TREE_SIDE_EFFECTS if its argument doesn't have it. No, this would defeat the purpose of the SAVE_EXPR, since you could duplicate the expression or move it at will, leading to nasty order of evaluation issues.
For: void f(){ int i = 2; (i+i); } we always set TREE_NOWARNING in finish_parenthesized_expr. The comment next to it says this is to avoid warning in c_common_truthvalue_conversion. If that's the only reason, the hammer seems a bit heavy, in C we only set the bit for MODIFY_EXPR.
Created attachment 31342 [details] patch The __builtin_shuffle part of the patch seems fine. For (i+i), I first tried not setting TREE_NOWARNING on things that are not MODIFY_EXPR (as in the C front-end), but apparently that's how the warning for logical ops is disabled for a || (b && c), so that's not ok. The current patch breaks g++.dg/ext/vla13.C (PR 54583), but nothing else covered by the testsuite, so it is tempting to see if there are other ways to fix PR 54583.
(In reply to Eric Botcazou from comment #5) > > For __builtin_shuffle, the issue is that we now call save_expr, which always > > sets TREE_SIDE_EFFECTS to 1. I don't know if it would make sense to > > introduce a maybe_save_expr that is equivalent to save_expr but does not set > > TREE_SIDE_EFFECTS if its argument doesn't have it. > > No, this would defeat the purpose of the SAVE_EXPR, since you could > duplicate the expression or move it at will, leading to nasty order of > evaluation issues. What I meant was, there are many places in the front-end that do: if (TREE_SIDE_EFFECTS (op0)) op0 = save_expr (op0); and I'd like to replace it with op0 = maybe_save_expr (op0); that would do the same thing to begin with. Now, if op0 is a long expression without side-effects, the code I quoted will not call save_expr, it will duplicate the expression and count on CSE for dedup, whereas we could imagine having a single version to start with, that optimizations would still be allowed to duplicate as usual if they consider it worth it. Of course the uses of SAVE_EXPR that require the stronger protection would be left alone. I may still not be making sense, but at least I think I was a little more precise about what I meant ;-)
> What I meant was, there are many places in the front-end that do: > > if (TREE_SIDE_EFFECTS (op0)) op0 = save_expr (op0); > > and I'd like to replace it with > > op0 = maybe_save_expr (op0); > > that would do the same thing to begin with. Now, if op0 is a long expression > without side-effects, the code I quoted will not call save_expr, it will > duplicate the expression and count on CSE for dedup, whereas we could > imagine having a single version to start with, that optimizations would > still be allowed to duplicate as usual if they consider it worth it. Yes, we do something like that in the Ada front-end, i.e. we call save_expr for complex expressions without side-effects, see e.g. utils2.c:gnat_protect_expr. But once the SAVE_EXPR is built, you *cannot* remove TREE_SIDE_EFFECTS on it, or else you'll run into nasty order of evaluation issues.
(In reply to Marc Glisse from comment #7) > The __builtin_shuffle part of the patch seems fine. Yes, that looks right. That fixes the bug, yes? The maybe_save_expr idea also makes sense to me, but doesn't seem necessary to fix this.
(In reply to Jason Merrill from comment #10) > (In reply to Marc Glisse from comment #7) > > The __builtin_shuffle part of the patch seems fine. > > Yes, that looks right. That fixes the bug, yes? It fixes this half of the bug, yes. I'll regtest it separately.
Author: glisse Date: Fri Jan 3 21:12:48 2014 New Revision: 206325 URL: http://gcc.gnu.org/viewcvs?rev=206325&root=gcc&view=rev Log: 2014-01-03 Marc Glisse <marc.glisse@inria.fr> PR c++/58950 gcc/cp/ * cvt.c (convert_to_void): Handle VEC_PERM_EXPR and VEC_COND_EXPR. gcc/testsuite/ * g++.dg/pr58950.C: New file. Added: trunk/gcc/testsuite/g++.dg/pr58950.C (with props) Modified: trunk/gcc/cp/ChangeLog trunk/gcc/cp/cvt.c trunk/gcc/testsuite/ChangeLog Propchange: trunk/gcc/testsuite/g++.dg/pr58950.C ('svn:eol-style' added) Propchange: trunk/gcc/testsuite/g++.dg/pr58950.C ('svn:keywords' added)
Testcase for the remaining part of the PR: void f(){ int i=2; (i+i); // should warn with -Wall }
(In reply to Marc Glisse from comment #7) > The current > patch breaks g++.dg/ext/vla13.C (PR 54583), but nothing else covered by the > testsuite, so it is tempting to see if there are other ways to fix PR 54583. Paolo, do you have an opinion on this PR?
I don't think you simply want a better fix for 54583, because for the testcase in #Comment 13 the new conditional setting TREE_NO_WARNING isn't used. Otherwise, I think it would be easy to tighten it via array_of_runtime_bound_p.
(In reply to Paolo Carlini from comment #15) > I don't think you simply want a better fix for 54583, because for the > testcase in #Comment 13 the new conditional setting TREE_NO_WARNING isn't > used. Otherwise, I think it would be easy to tighten it via > array_of_runtime_bound_p. The issue isn't with setting the bit but reading it. If you look at the patch, I remove a test for TREE_NO_WARNING (expr). This breaks 54583 because the TREE_NO_WARNING bit is then ignored.
Yes, I know that. What I'm saying is that other code may want to see that TREE_NO_WARNING honored, the issue doesn't have much to do with 54583 per se. In my personal opinion removing a TREE_NO_WARNING check is in general a pretty risky thing to do, because unfortunately we have only that generic bit and we use it in many different circumstances.
GCC 4.9.0 has been released
GCC 4.9.1 has been released.
GCC 4.9.2 has been released.
Should this be P1?
GCC 4.9.3 has been released.
Given that Comment #1 clearly shows g++ 4.8 working, I think this should qualify as a regression. Moreover, g++ 6.2 works correctly, so this was fixed at some point between 4.9 and 6.2: $ cat > a.cc void f() { int i __attribute__((vector_size(2*sizeof(int))))={2,3}; __builtin_shuffle(i,i); } $ g++ -c a.cc -Wunused-value a.cc: In function 'void f()': a.cc:3:3: warning: value computed is not used [-Wunused-value] __builtin_shuffle(i,i); ^~~~~~~~~~~~~~~~~
Ah.. I missed Comment #13, the PR is still open because of a slightly different test. In any case, if it worked in 4.8, it should be a regression.
(In reply to Marc Glisse from comment #6) > For: > > void f(){ > int i = 2; > (i+i); > } This case is fixed on the trunk: <source>: In function 'void f1()': <source>:9:5: warning: statement has no effect [-Wunused-value] 9 | (i+i); | ~~^~~
(In reply to Andrew Pinski from comment #25) > (In reply to Marc Glisse from comment #6) > > For: > > > > void f(){ > > int i = 2; > > (i+i); > > } > > This case is fixed on the trunk: > > <source>: In function 'void f1()': > <source>:9:5: warning: statement has no effect [-Wunused-value] > 9 | (i+i); > | ~~^~~ I think it was fixed via r12-1804-g65870e75616ee4359d1c13b99 .