void f(int**p,int**q){ ++**p; *q=*p; --**p; } produces _1 = *p_8(D); _2 = *_1; _3 = _2 + 1; *_1 = _3; *q_10(D) = _1; _4 = *p_8(D); etc where we do not CSE _4 -> _1. We do have code in vn_reference_lookup_3 to handle this kind of thing, look past a potential clobber, and ignore it if we find the same value. But for this test, vn_reference_lookup_2 returns 0 and we never look further than *q=1 when we valueize *p for _4. If we did, I expect we would skip *_1=3 using TBAA and eventually find _1=*p, which matches the potential clobber. (this comes from the same code as PR 93745 but should not have the same issue)
On the original code (I can attach it if needed, but it is large, it is resizing a std::vector with reference-counted elements) FRE3 fails to simplify MEM[(struct Handle_for *)__cur_16] ={v} {CLOBBER}; _17 = MEM[(const struct Handle_for &)__first_15].ptr_; MEM[(struct Handle_for *)__cur_16].ptr_ = _17; _18 = *_17.count; _19 = _18 + 1; *_17.count = _19; _20 = &__first_15->D.202020; _31 = MEM[(struct Handle_for *)__first_15].ptr_; while FRE4 sees MEM[(struct Handle_for *)__cur_3] ={v} {CLOBBER}; _17 = MEM[base: __first_20, offset: 0B]; MEM[base: __cur_3, offset: 0B] = _17; _18 = *_17.count; _19 = _18 + 1; *_17.count = _19; _31 = MEM[base: __first_20, offset: 0B]; and replaces _31 with _17. That's confusing, since the main difference between the 2 is removing a statement without VOP. (optimizing in FRE4 is way too late in this case, I want the simplified version before ldist, and it still requires at least DSE, some pass detecting a self-assignment, and DCE before that) Here is another simplified version of the testcase, but you need to compile it with -fno-early-inlining to see the issue: void g(); struct A { int*p; A(A const&a)noexcept:p(a.p){if(*p<=0)__builtin_unreachable();++*p;} ~A(){if(--*p==0)g();} }; #include <vector> void f(std::vector<A>&v){ v.reserve(1<<20); } At the end of gimple, we still have _92 = *_91; *_91 = _92; in the main loop, while I would want that gone before ldist.
I'll have a look somewhen. I specifically made the code in vn_reference_lookup_3 not do any walking due to cost considerations (not wanting to "recurse").
So FRE/PRE see _91 = __MEM <const struct A> ((const struct A &)__first_58).p; __MEM <struct A> (__cur_59).p = _91; _92 = __MEM <int> (_91); if (_92 <= 0) goto __BB5(precise(0)); else goto __BB6(precise(134217728)); __BB(5,precise(0)): __builtin_unreachable (); __BB(6,guessed_local(-1430369669)): _93 = _92 + 1; __MEM <int> (_91) = _93; _94 = __first_58->p; _95 = __MEM <int> (_94); _96 = _95 + _Literal (int) -1; __MEM <int> (_94) = _96; but FRE4 sees _91 = MEM[base: __first_58, offset: _Literal (int * *) 0]; MEM[base: __cur_59, offset: _Literal (int * *) 0] = _91; _92 = __MEM <int> (_91); if (_92 <= 0) goto __BB5(precise(0)); else goto __BB6(precise(134217728)); __BB(5,precise(0)): __builtin_unreachable (); __BB(6,guessed_local(-1430369669)): _93 = _92 + 1; __MEM <int> (_91) = _93; _94 = MEM[base: __first_58, offset: _Literal (int * *) 0]; _95 = __MEM <int> (_94); _96 = _95 + _Literal (int) -1; __MEM <int> (_94) = _96; which might be enough different TBAA-wise to have the intermediate def stmt skipped. Ah, so we don't do if (is_gimple_reg_type (TREE_TYPE (lhs)) && types_compatible_p (TREE_TYPE (lhs), vr->type) && ref->ref) because we've valueized something in __first_75->sp and thus ref->ref is NULL and we need ref->ref for the alignment check. But we can use data->orig_ref.ref then. That doesn't help it seems because the load was entered differently in the hash table (a different vuse via the last_vuse mechanism - that's the immediately preceeding clobber we disambiguated against). Ideally the hash tables would contain entries for all virtual uses the expression is valid in but that's somewhat hard to encode (well, maybe simply not hash the vuse and record orig and last vuse and then do dominance checks to see if a query vuse falls in range). First piece of a fix: diff --git a/gcc/tree-ssa-sccvn.c b/gcc/tree-ssa-sccvn.c index a3fba2878f8..5d78be7e9d3 100644 --- a/gcc/tree-ssa-sccvn.c +++ b/gcc/tree-ssa-sccvn.c @@ -2455,7 +2455,7 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_, and return the found value. */ if (is_gimple_reg_type (TREE_TYPE (lhs)) && types_compatible_p (TREE_TYPE (lhs), vr->type) - && ref->ref) + && (ref->ref || data->orig_ref.ref)) { tree *saved_last_vuse_ptr = data->last_vuse_ptr; /* Do not update last_vuse_ptr in vn_reference_lookup_2. */ @@ -2480,7 +2480,9 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_, -fno-strict-aliasing. So simply resort to alignment to rule out overlaps. Do this check last because it is quite expensive compared to the hash-lookup above. */ - && multiple_p (get_object_alignment (ref->ref), ref->size) + && multiple_p (get_object_alignment + (ref->ref ? ref->ref : data->orig_ref.ref), + ref->size) && multiple_p (get_object_alignment (lhs), ref->size)) return res; }
While the last_vuse thing was originally added for PRE it's also useful for FRE as the following testcase shows: int foo(int *p, int b, float *q) { int tem; if (b) { *q = 0; tem = *p; } else { *q = 1; tem = *p; } return *p - tem; } with last_vuse fre1 manages to optimize this to return 0 while w/o it doesn't (the key is value-numbering loads from both if arms the same).
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>: https://gcc.gnu.org/g:367766f40a031ff064857681dc4da3309f0ce57d commit r11-41-g367766f40a031ff064857681dc4da3309f0ce57d Author: Richard Biener <rguenther@suse.de> Date: Tue Feb 25 11:46:02 2020 +0100 tree-optimization/93891 - improve same-store disambiguation We need a reference to assess alignment, fall back to the original reference tree if available. 2020-05-04 Richard Biener <rguenther@suse.de> PR tree-optimization/93891 * tree-ssa-sccvn.c (vn_reference_lookup_3): Fall back to the original reference tree for assessing access alignment.
Not yet fixed.
Re-confirmed. The skipping logic only handles constants in full generality and non-constants only over a single hop, but there's an intervening unrelated store in the way. Generalizing the ->same_val code to handle non-constants might work (I do not remmeber why I didn't do that yet).