On 01/10/15 17:18, Marek Polacek wrote: > On Thu, Oct 01, 2015 at 11:02:09AM -0400, David Edelsohn wrote: >> On Thu, Oct 1, 2015 at 10:49 AM, Marek Polacek <pola...@redhat.com> wrote: >>> Joseph reminded me that I had forgotten about this patch. As mentioned >>> here <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01792.html>, I'm >>> removing the XFAILs in the tests so people are likely to see new FAILs. >>> >>> I think the following targets will need similar fix as the one below: >>> * MIPS >>> * rs6000 >>> * alpha >>> * sparc >>> * s390 >>> * arm >>> * sh >>> * aarch64 >>> >>> I'm CCing the respective maintainers. You might want to XFAIL those tests.
Thanks for the heads up. The use of the _Atomic feature is not really target specific but more a language standards issue across all the architectures that the GCC project supports, therefore XFAILing them is the wrong approach imho. >> >> Why aren't you testing the appropriate fix on all of the targets? > > It's very improbable that I could fix and properly test all of them; > I simply don't have the cycles and resources to fix e.g. sh/sparc/alpha/mips. I don't think anyone expects you to be testing the patch on every single port ..... Even though these changes sit in the target hooks into various backends, you may be best placed to advise how target maintainers adjust their backends. If at that point this appears to be mechanical, it's been good practice in the community for folks to send patches that the maintainers can fully test even if the testing has been light for the proposed patch. However, I am not aware of a "policy" for these things other than that these sort of changes are selectively enforced in the community. Maybe we should think about it .... > > You want me to revert my fix, but I don't really see the point here; the > patch doesn't introduce any regressions, it's just that the new tests are > likely to FAIL. It sounds preferable to me to fix 2 targets than to leave > all of them broken (and I bet many maintainers were unaware of the issue). > > Would XFAILing the new tests work for you, if you don't want to see any > new FAILs? > > If you still insist on reverting the patch, ok, but I think this PR is > unlikely to be resolved any time soon then. > > Marek > I've had a quick look on aarch64 - changing the interface to use create_tmp_var_raw is rather mechanical. What I'm struggling with is figuring out whether the change for TARGET_EXPR is applicable in the arm / aarch64 backends. It took me a couple of minutes to trial the interface changes (attached) on aarch64 as I had a cross-compiler build tree lying around and could see that the compiler did not ICE with the 2 testcases provided and pr65345-4.c appeared to pass on hardware. regards Ramana
diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c index 80916a9..716ed6e 100644 --- a/gcc/config/aarch64/aarch64-builtins.c +++ b/gcc/config/aarch64/aarch64-builtins.c @@ -1462,8 +1462,8 @@ aarch64_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update) __builtin_aarch64_set_cr (masked_cr); __builtin_aarch64_set_sr (masked_sr); */ - fenv_cr = create_tmp_var (unsigned_type_node); - fenv_sr = create_tmp_var (unsigned_type_node); + fenv_cr = create_tmp_var_raw (unsigned_type_node); + fenv_sr = create_tmp_var_raw (unsigned_type_node); get_fpcr = aarch64_builtin_decls[AARCH64_BUILTIN_GET_FPCR]; set_fpcr = aarch64_builtin_decls[AARCH64_BUILTIN_SET_FPCR]; @@ -1476,9 +1476,9 @@ aarch64_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update) ~(AARCH64_FE_ALL_EXCEPT)); ld_fenv_cr = build2 (MODIFY_EXPR, unsigned_type_node, - fenv_cr, build_call_expr (get_fpcr, 0)); + fenv_cr, build_call_expr (get_fpcr, 0)); ld_fenv_sr = build2 (MODIFY_EXPR, unsigned_type_node, - fenv_sr, build_call_expr (get_fpsr, 0)); + fenv_sr, build_call_expr (get_fpsr, 0)); masked_fenv_cr = build2 (BIT_AND_EXPR, unsigned_type_node, fenv_cr, mask_cr); masked_fenv_sr = build2 (BIT_AND_EXPR, unsigned_type_node, fenv_sr, mask_sr); @@ -1509,7 +1509,7 @@ aarch64_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update) __atomic_feraiseexcept (new_fenv_var); */ - new_fenv_var = create_tmp_var (unsigned_type_node); + new_fenv_var = create_tmp_var_raw (unsigned_type_node); reload_fenv = build2 (MODIFY_EXPR, unsigned_type_node, new_fenv_var, build_call_expr (get_fpsr, 0)); restore_fnenv = build_call_expr (set_fpsr, 1, fenv_sr);