Hi Yuao,
Yuao Ma wrote:
I believe the attached patch should be safe to remove now that
r16-4474-g2c1949bf152f8f has landed. We can confirm this by checking
the codegen: https://godbolt.org/z/fvo1PG3bs. The output for the
current trunk shows that the comp is no longer being emitted.
Can you verify by adding a 'gcc_assert(false)' to the
condition and run the GCC testsuite instead of just picking
a random test?
The testsuite/gfortran.dg/coarray/ tests are run with -fcoarray=single
and -fcoarray=lib (albeit only with the stub -lcaf_single).
On the testcase base, atomic_cas is (only) used in
gcc/testsuite/gfortran.dg/coarray/atomic_{2,3}.f90
* * *
Having said this, I am pretty sure that it is fine and ...
Regression tested on x86_64-linux.
... should have also found issues.
* * *
This part is unreachable after r16-4474-g2c1949bf152f8f.
gcc/fortran/ChangeLog:
* intrinsic.texi: Fix typo.
* trans-intrinsic.cc (conv_intrinsic_atomic_cas): Remove unreachable
code.
LGTM.
Thanks for the cleanup.
* * *
Regarding:
While working on this, I noticed a part of the code that I think we
can simplify:
```
if (TREE_TYPE (TREE_TYPE (new_val)) != TREE_TYPE (TREE_TYPE (old)))
{
tmp = gfc_create_var (TREE_TYPE (TREE_TYPE (old)), "new");
gfc_add_modify (&block, tmp, fold_convert (TREE_TYPE (tmp), new_val));
new_val = gfc_build_addr_expr (NULL_TREE, tmp);
}
```
I suspect this conversion is unnecessary
I believe this is effectively the same as:
+ /* Convert a constant to a pointer. */
+ if (!POINTER_TYPE_P (TREE_TYPE (comp))
even though it handles in principle more.
Thus, this code was needed before your patch, but handled only a subset
of what the comment claimed.
* * *
for a couple of reasons:
1. Fortran 2018 and 2023 standards require the new argument to have
the same type and kind.
2. Even with older standards that might have been more lenient (like
possibly 2008), we typically only support one valid kind for different
types: ATOMIC_INT_KIND for integer and ATOMIC_LOGICAL_KIND for
logical.
Coarrays existed in the Cray compiler and some university compiler
before, but with some differences. Fortran 2018 was the first
version that added coarrays, but a technical specification
(TS18508) existed before. It is no longer available for free at
its previous location and I don't feel like digging for it.
But I am rather certain that it also required this, not at least
because only ATOMIC_INT_KIND and ATOMIC_LOGICAL_KIND are supported,
it is hard to pass anything else ...
For F2018, has:
OLD, COMPARE, and NEW "shall be scalar and of the same type and kind as ATOM".
* * *
My plan is to update the documentation to align with the latest
standard, remove this part for the new argument here, and add a check
to reject different kinds during resolving.
Thanks for updating the documentation. Regarding the check,
the one in check.cc's gfc_check_atomic_cas should already
cover this. It seems to be there once atomic_cas was added,
namely in commit r5-1806-g7f4aaf912bdab4.
The next commit added the library support part for it and
the code in question: r5-1807-g42a8246dbdb963
* * *
Since I'm not entirely 100% sure about this, I think it could be a
seperate patch.
Given that the current patch is fine, I would do it as extra patch.
As mentioned, I'd recommend to add a gcc_assert(false) or gfc_error or …
in code that is to be removed - and and run the testsuite to see
where/whether it breaks.
However, I am pretty sure that it won't break (because of the check)
and that is was in the code effectively only because of the const
issue, even though it could in theory handle more.
Thanks,
Tobias
