> On Mon, 31 Mar 2025, Martin Jambor wrote: > > > Hi, > > > > the testcase in PR 118924, when compiled on Aarch64, contains an > > gimple aggregate assignment statement in between different types which > > are types_compatible_p but behave differently for the purposes of > > alias analysis. > > > > SRA replaces the statement with a series of scalar assignments which > > however have LHSs access chains modeled on the RHS type and so do not > > alias with a subsequent reads and so are DSEd. > > > > SRA clearly gets its "same_access_path" logic subtly wrong. One issue > > is that the same_access_path_p function probably should be implemented > > more along the lines of (parts of ao_compare::compare_ao_refs) instead > > of internally relying on operand_equal_p. That is however not the > > problem in the PR and so I will deal with it only later. > > > > The issue here is that even when the access path is the same, it must > > not be bolted on an aggregate type that does not match. This patch > > does that, taking just one simple function from the > > ao_compare::compare_ao_refs machinery and using it to detect the > > situation. The rest is just merging the information in between > > accesses of the same access group. > > > > I looked at how many times we come across such assignment during > > "make stage2-bubble" of GCC (configured with only c and C++ and > > without multilib and libsanitizers) and on an x86_64 there were 87924 > > such assignments (though now I realize not all of them had to be > > aggregate), so they do happen. The patch leads to about 5% increase > > of cases where we don't use an "access path" but resort to a > > MEM_REF (from 90209 to 95204). On an Aarch64, there were 92268 such > > assignments and the increase of falling back to MEM_REFs was by > > 4% (but from a bigger base 132983 to 107991). > > > > Bootstrapped on x86_64-linux and Aarch64-linux. OK for master and then > > for all active release branches? > > I'm unsure about the lto_streaming_safe arg, in fact the implementation is > > if (lto_streaming_safe) > return type1 == type2; > else > return TYPE_CANONICAL (type1) == TYPE_CANONICAL (type2); > > but I think we guarantee (we _need_ to guarantee!) that when > TYPE_CANONICAL (type1) == TYPE_CANONICAL (type2), after LTO streaming > it's still TYPE_CANONICAL (type1) == TYPE_CANONICAL (type2). Otherwise > assignments previously valid GIMPLE might no longer be valid and > things that aliased previously might no longer alias.
The reason why we can not test TYPE_CANONICAL equivalence here is the way ODR types work. Suppose we have type A, B and C which are structurally equiavalent and suppose that A, B has ODR name, while C is from non-C++ source w/o ODR. Type merging will notice ODR/non-ODR conflict and type canonical will be the same. Now the program will get partitioned and if partition uses A and B, but not C, the canonical type computation in ltrans will have different canonicals for A and B. So in WPA we can not assume that TYPE_CANONICAL (A) == TYPE_CANONICAL (B) is forever. We also don't do any gimple transforms here, so this is kind of safe, but ugly. I think types_equal_for_same_type_for_tbaa_p can simply use flag_wpa and we can drop that parameter? I guess that would be less confusing, since the name is kind of misleading anyway. If the fact that WPA may see worse TBAA then ltrans will end up hurting in pratcice we may add extra flag to ODR types when canonical type computation should ignore it and stream it WPA->ltrans to disable the extra precision. Honza
