Following code has been produced via reduction with `creduce`. When compiled with `-O2`, GCC 11 and later versions will incorrectly print `f`, while if `-O1` or lower, or an older version of GCC is used, it will correctly print 't'. #include <algorithm> #include <cstdio> #include <optional> inline std::optional<int> a(std::vector<int>::iterator b, std::vector<int>::iterator c, std::optional<int> h(int)) { std::optional<int> d; find_if(b, c, [&](auto e) { d = h(e); return d; }); return d; } std::optional<int> f(int) { return 1; } main() { std::vector<int> g(100); auto e = a(g.begin(), g.end(), f); printf("%c", e ? 't' : 'f'); } For the sake of completion, this was the original code: https://godbolt.org/z/enx19v7E5
From fre3 (with details): Value numbering stmt = *__pred$__d_53 = _58 (); Setting value number of .MEM_143 to .MEM_135 (changed) Value numbering stmt = SR.60_59 = MEM <unsigned char> [(const struct optional &)__pred$__d_53 + 4]; Setting value number of SR.60_59 to 0 (changed) Value numbering stmt = _60 = VIEW_CONVERT_EXPR<bool>(SR.60_59); Match-and-simplified VIEW_CONVERT_EXPR<bool>(SR.60_59) to 0 RHS VIEW_CONVERT_EXPR<bool>(SR.60_59) simplified to 0 Hmm, Somehow *__pred$__d_53 is missed.
This changed with r11-3408-ge977dd5edbcc3a3b88c3bd7efa1026c845af7487
Testcase without the unneeded <cstdio> which aborts if miscompiled. #include <algorithm> #include <optional> inline std::optional<int> a(std::vector<int>::iterator b, std::vector<int>::iterator c, std::optional<int> h(int)) { std::optional<int> d; find_if(b, c, [&](auto e) { d = h(e); return d; }); return d; } std::optional<int> f(int) { return 1; } int main() { std::vector<int> g(100); auto b = g.begin(); auto c = g.end(); auto e = a(b, c, f); if (!e) __builtin_abort(); }
(In reply to Jakub Jelinek from comment #2) > This changed with r11-3408-ge977dd5edbcc3a3b88c3bd7efa1026c845af7487 Hmm, even -fno-ipa-modref does not prevent the wrong code from showing up.
But adding noipa to f does though: [[gnu::noipa]] std::optional<int> f() { return 1; }
-O2 -fno-ipa-pure-const helps though and the patch affected both.
And it ICEs again with -O2 -fno-ipa-pure-const if I add [[gnu::const]] or [[gnu::pure]] attribute to f. The function seems to be const in that it only returns the result and doesn't access any other memory, but the return value does not have a gimple reg type (but is returned in registers on x86_64): _Z1fi: movl $1, -8(%rsp) movb $1, -4(%rsp) movq -8(%rsp), %rax ret And we really should do something to make it movabsq $4294967297, %rax ret or so.
I believe the bug is in visit_reference_op_call: 5241 /* If we value numbered an indirect functions function to 5242 one not clobbering memory value number its VDEF to its 5243 VUSE. */ 5244 tree fn = gimple_call_fn (stmt); 5245 if (fn && TREE_CODE (fn) == SSA_NAME) 5246 { 5247 fn = SSA_VAL (fn); 5248 if (TREE_CODE (fn) == ADDR_EXPR 5249 && TREE_CODE (TREE_OPERAND (fn, 0)) == FUNCTION_DECL 5250 && (flags_from_decl_or_type (TREE_OPERAND (fn, 0)) 5251 & (ECF_CONST | ECF_PURE))) 5252 vdef_val = vuse_ssa_val (gimple_vuse (stmt)); 5253 } 5254 changed |= set_ssa_val_to (vdef, vdef_val); stmt in this case is: # .MEM_140 = VDEF <.MEM_96> *__pred$__d_43 = _50 (_49); and _50 value numbers to &f where f is a const function. I think we can value number the vdef to vuse_ssa_val (gimple_vuse (stmt)) only if the lhs of the call is not present or is an SSA_NAME, when the call (const or pure) acts as a store, we shouldn't do that. A few lines before this there is also: if (vnresult->result_vdef && vdef) changed |= set_ssa_val_to (vdef, vnresult->result_vdef); else if (vdef) /* If the call was discovered to be pure or const reflect that as far as possible. */ changed |= set_ssa_val_to (vdef, vuse_ssa_val (gimple_vuse (stmt))); I wonder if that doesn't suffer from the same problem.
But --- gcc/tree-ssa-sccvn.cc.jj 2022-02-11 00:19:22.432063254 +0100 +++ gcc/tree-ssa-sccvn.cc 2022-02-23 16:07:36.697893998 +0100 @@ -5218,7 +5218,11 @@ visit_reference_op_call (tree lhs, gcall if (vnresult) { - if (vnresult->result_vdef && vdef) + /* If stmt has non-SSA_NAME lhs, don't value number the vdef, + as the call still acts as a lhs store. */ + if (!lhs && gimple_call_lhs (stmt)) + ; + else if (vnresult->result_vdef && vdef) changed |= set_ssa_val_to (vdef, vnresult->result_vdef); else if (vdef) /* If the call was discovered to be pure or const reflect @@ -5248,7 +5252,10 @@ visit_reference_op_call (tree lhs, gcall if (TREE_CODE (fn) == ADDR_EXPR && TREE_CODE (TREE_OPERAND (fn, 0)) == FUNCTION_DECL && (flags_from_decl_or_type (TREE_OPERAND (fn, 0)) - & (ECF_CONST | ECF_PURE))) + & (ECF_CONST | ECF_PURE)) + /* If stmt has non-SSA_NAME lhs, don't value number the + vdef, as the call still acts as a lhs store. */ + && (lhs || gimple_call_lhs (stmt) == NULL_TREE)) vdef_val = vuse_ssa_val (gimple_vuse (stmt)); } changed |= set_ssa_val_to (vdef, vdef_val); results in ICEs later on - the gcc_assert (x != VN_TOP); assert in vuse_ssa_val.
But: --- gcc/tree-ssa-sccvn.cc.jj 2022-02-11 00:19:22.432063254 +0100 +++ gcc/tree-ssa-sccvn.cc 2022-02-23 16:16:25.873557626 +0100 @@ -5218,7 +5218,11 @@ visit_reference_op_call (tree lhs, gcall if (vnresult) { - if (vnresult->result_vdef && vdef) + /* If stmt has non-SSA_NAME lhs, don't value number the vdef, + as the call still acts as a lhs store. */ + if (!lhs && vdef && gimple_call_lhs (stmt)) + changed |= set_ssa_val_to (vdef, vdef); + else if (vnresult->result_vdef && vdef) changed |= set_ssa_val_to (vdef, vnresult->result_vdef); else if (vdef) /* If the call was discovered to be pure or const reflect @@ -5248,7 +5252,10 @@ visit_reference_op_call (tree lhs, gcall if (TREE_CODE (fn) == ADDR_EXPR && TREE_CODE (TREE_OPERAND (fn, 0)) == FUNCTION_DECL && (flags_from_decl_or_type (TREE_OPERAND (fn, 0)) - & (ECF_CONST | ECF_PURE))) + & (ECF_CONST | ECF_PURE)) + /* If stmt has non-SSA_NAME lhs, don't value number the + vdef, as the call still acts as a lhs store. */ + && (lhs || gimple_call_lhs (stmt) == NULL_TREE)) vdef_val = vuse_ssa_val (gimple_vuse (stmt)); } changed |= set_ssa_val_to (vdef, vdef_val); works for the testcase.
Created attachment 52500 [details] gcc12-pr104601.patch Full untested patch.
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>: https://gcc.gnu.org/g:9251b457eb8df912f2e8203d0ee8ab300c041520 commit r12-7371-g9251b457eb8df912f2e8203d0ee8ab300c041520 Author: Jakub Jelinek <jakub@redhat.com> Date: Thu Feb 24 15:29:02 2022 +0100 sccvn: Fix visit_reference_op_call value numbering of vdefs [PR104601] The following testcase is miscompiled, because -fipa-pure-const discovers that bar is const, but when sccvn during fre3 sees # .MEM_140 = VDEF <.MEM_96> *__pred$__d_43 = _50 (_49); where _50 value numbers to &bar, it value numbers .MEM_140 to vuse_ssa_val (gimple_vuse (stmt)). For const/pure calls that return a SSA_NAME (or don't have lhs) that is fine, those calls don't store anything, but if the lhs is present and not an SSA_NAME, value numbering the vdef to anything but itself means that e.g. walk_non_aliased_vuses won't consider the call, but the call acts as a store to its lhs. When it is ignored, sccvn will return whatever has been stored to the lhs earlier. I've bootstrapped/regtested an earlier version of this patch, which did the if (!lhs && gimple_call_lhs (stmt)) changed |= set_ssa_val_to (vdef, vdef); part before else if (vnresult->result_vdef), and that regressed +FAIL: gcc.dg/pr51879-16.c scan-tree-dump-times pre "foo \\\\(" 1 +FAIL: gcc.dg/pr51879-16.c scan-tree-dump-times pre "foo2 \\\\(" 1 so this updated patch uses result_vdef there as before and only otherwise (which I think must be the const/pure case) decides based on whether the lhs is non-SSA_NAME. 2022-02-24 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/104601 * tree-ssa-sccvn.cc (visit_reference_op_call): For calls with non-SSA_NAME lhs value number vdef to itself instead of e.g. the vuse value number. * g++.dg/torture/pr104601.C: New test.
Fixed on the trunk so far.
The releases/gcc-11 branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>: https://gcc.gnu.org/g:d29a0b50687dc42e17dc4a4fe335afafefeada4e commit r11-9718-gd29a0b50687dc42e17dc4a4fe335afafefeada4e Author: Jakub Jelinek <jakub@redhat.com> Date: Thu Feb 24 15:29:02 2022 +0100 sccvn: Fix visit_reference_op_call value numbering of vdefs [PR104601] The following testcase is miscompiled, because -fipa-pure-const discovers that bar is const, but when sccvn during fre3 sees # .MEM_140 = VDEF <.MEM_96> *__pred$__d_43 = _50 (_49); where _50 value numbers to &bar, it value numbers .MEM_140 to vuse_ssa_val (gimple_vuse (stmt)). For const/pure calls that return a SSA_NAME (or don't have lhs) that is fine, those calls don't store anything, but if the lhs is present and not an SSA_NAME, value numbering the vdef to anything but itself means that e.g. walk_non_aliased_vuses won't consider the call, but the call acts as a store to its lhs. When it is ignored, sccvn will return whatever has been stored to the lhs earlier. I've bootstrapped/regtested an earlier version of this patch, which did the if (!lhs && gimple_call_lhs (stmt)) changed |= set_ssa_val_to (vdef, vdef); part before else if (vnresult->result_vdef), and that regressed +FAIL: gcc.dg/pr51879-16.c scan-tree-dump-times pre "foo \\\\(" 1 +FAIL: gcc.dg/pr51879-16.c scan-tree-dump-times pre "foo2 \\\\(" 1 so this updated patch uses result_vdef there as before and only otherwise (which I think must be the const/pure case) decides based on whether the lhs is non-SSA_NAME. 2022-02-24 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/104601 * tree-ssa-sccvn.c (visit_reference_op_call): For calls with non-SSA_NAME lhs value number vdef to itself instead of e.g. the vuse value number. * g++.dg/torture/pr104601.C: New test. (cherry picked from commit 9251b457eb8df912f2e8203d0ee8ab300c041520)
Fixed for 11.3 too.