On Sun, Jan 11, 2015 at 1:39 PM, Bernd Edlinger
<[email protected]> 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.
>
