On Sun, Jan 11, 2015 at 1:39 PM, Bernd Edlinger <bernd.edlin...@hotmail.de> wrote: > Hi Richard, > > On Fri, 9 Jan 2015 17:19:57, Richard Biener wrote: >> >> >> Yes. As said, you generally need to run folding results through >> force_gimple_operand. >> >> Richard. >> > > > I have now used force_gimple_operand instead of special casing the > VIEW_CONVERT_EXPRs. > And I see that all Ada test cases still work with -fsanitize=thread. So this > feels like an improvement. > > I have checked with a large C++ application, to see if the generated code > changes or not. > And although this looked like it should not change the resulting code, I > found one small difference > at -O3 -fsanitize=thread while compiling the function > xmlSchemaCompareValuesInt in xmlschematypes.c > of libxml2. The generated code size did not change, only two blocks of code > changed place. > That was the only difference in about 16 MB of code. > > The reason for this seems to be the following changes in the > xmlschemastypes.c.104t.tsan1 > > <bb 29>: > p1_179 = xmlSchemaDateNormalize (x_7(D), 0.0); > # DEBUG p1 => p1_179 > _180 = _xmlSchemaDateCastYMToDays (p1_179); > - _660 = &p1_179->value.date; > - _659 = &MEM[(struct xmlSchemaValDate *)_660 + 8B]; > - __builtin___tsan_read2 (_659); > + _660 = &MEM[(struct xmlSchemaValDate *)p1_179 + 24B]; > + __builtin___tsan_read2 (_660); > _181 = p1_179->value.date.day; > _182 = (long int) _181; > p1d_183 = _180 + _182; > > this pattern is repeated everywhere. (- = before the patch. + = with the > patch) > > So it looks as if the generated code quality slightly improves with this > change. > > I have also tried to fold &base + offset + bitpos, like this: > > --- tsan.c.orig 2015-01-10 00:39:06.465210937 +0100 > +++ tsan.c 2015-01-11 09:28:38.109423856 +0100 > @@ -213,7 +213,18 @@ instrument_expr (gimple_stmt_iterator gs > align = get_object_alignment (expr); > if (align < BITS_PER_UNIT) > return false; > - expr_ptr = build_fold_addr_expr (unshare_expr (expr)); > + expr_ptr = build_fold_addr_expr (unshare_expr (base)); > + if (bitpos != 0) > + { > + if (offset != NULL) > + offset = size_binop (PLUS_EXPR, offset, > + build_int_cst (sizetype, > + bitpos / BITS_PER_UNIT)); > + else > + offset = build_int_cst (sizetype, bitpos / BITS_PER_UNIT); > + } > + if (offset != NULL) > + expr_ptr = fold_build_pointer_plus (expr_ptr, offset); > } > expr_ptr = force_gimple_operand (expr_ptr, &seq, true, NULL_TREE); > if ((size & (size - 1)) != 0 || size> 16 > > > For simplicity first only in the simple case without > DECL_BIT_FIELD_REPRESENTATIVE. > I tried this change at the same large C++ application, and see the code still > works, > but the binary size increases at -O3 by about 1%. > > So my conclusion would be that it is better to use force_gimple_operand > directly > on build_fold_addr_expr (unshare_expr (expr)), without using offset.
Yeah, it probably needs more investigation. > Well, I think this still resolves your objections. > > Furthermore I used may_be_nonaddressable_p instead of is_gimple_addressable > and just return if it is found to be not true. (That did not happen in my > tests.) > > And I reworked the block with the pt_solution_includes. > > I found that It can be rewritten, because pt_solution_includes can be > expanded to (is_global_var (decl) || pt_solution_includes_1 > (&cfun->gimple_df->escaped, decl) > || pt_solution_includes_1 (&ipa_escaped_pt, decl)) > > So, by De Morgan's law, you can rewite that block to > > if (DECL_P (base)) > { > if (!is_global_var (base) > && !pt_solution_includes_1 (&cfun->gimple_df->escaped, base) > && !pt_solution_includes_1 (&ipa_escaped_pt, base)) > return false; > if (!is_global_var (base) && !may_be_aliased (base)) > return false; > } > > Therefore I can move the common term !is_global_var (base) out of the block. > That's what I did. > As far as I can tell, none of the other terms here seem to be redundant. > > > Attached patch was boot-strapped and regression-tested on x86_64-linux-gnu. > OK for trunk? Ok. Thanks for these improvements! Richard. > > Thanks > Bernd. >