Hi PA,
Paul-Antoine Arras wrote:
While going through the Fortran review, I noticed that there was a
case working in C and not in C++:
#pragma omp target update to(arr[::2])
The literal 2 would get rejected because it was treated as an
id-expression. This is now fixed in
0001-WIP-C-fixup-for-non-id-expr.patch.
I think it is OK to accept it – but the question is whether
we need to. At a glance, the patch looks ok, though.
I will review it in a follow up email.
* * *
That C++ regards ':: <something>' as id-expression is not new
and matches what OpenMP expects, there are just a few special
cases that one may want to support or not:
* [: , :] and [::] - as C++26 splice operators aren't permitted in
OpenMP array sections and there is plenty of pre-C++26 code using it,
it should be handled. (GCC does and some patch of the strided-support
patches also does so)
* OpenMP 6 stated that [:::n] has to be read as '[: ::n]'
There is an OpenMP spec issue (Issue 4740) opened for strided but also
talking about :: for some the above and below issues.
* For [::::n] it was discussed that this is to be read as [: : ::n]
in line of the 6 wording above
* [::] is presumably to be regarded as [ : : ] and not as '::]' id expression.
* I just added ::2 (etc.) as follow up RFC to that issue.
* * *
Something that also came up as part of the Fortran review, but
actually belongs in the middle end, is the lack of a positive check
for a stride that is not known at compile time. This check is now
added in omp-low.cc and fires at run time whenever the stride is not a
compile-time constant. See
0002-ME-fixup-to-check-at-runtime-for-non-positive-stride.patch.
I am to sure we really need this.
We should really diagnose non-postive strides if known at compile time;
that's simple and we do so for C++ – but seemingly not yet for C or
Fortran.
(For C++ it was added in one of the follow up patches; as it is in a C/C++
testcase, I assume you have locally a patch for C.)
* * *
In terms of diagnostics, I think we have to distinguish between:
* Valid but not supported by the implementation
* Invalid user code (not trivially detectable at compile time)
In general, we should try to diagnose the former: Valid code might
break because the implementation is incomplete. Ideally, this
happens at compile time - even if we reject some actually supported
code - like: 'int n = 1;' + 'to(iterator(i=...): A[i][::n]' even if
the n == 1 is supported, if specified, it is likely n != 1.
But if we cannot do so at compile time, issues a runtime error is fine.
For pure user errors, we normally only do it when required by the
specification (if ... occurs, runtime termination ...). Otherwise,
we leave it to, e.g., -fcheck=... in Fortran or -fsanitize=undefined.
For this specific case, I wonder whether that's even invalid in general:
At a glance, the following does not seem to violate any restriction but
still leads to a negative stride (negative stride multiplier):
program main
implicit none
integer, allocatable :: A(:)
A = [1,2,3,4,5,6,7,8]
!$omp enter target data map(A) ! map contiguous memory as required
print *, A(8:1:-1) ! 8 7 6 5 4 3 2 1
call bar(A(8:1:-1))
contains
subroutine bar(X)
integer :: X(:)
print *, X(::2) ! 8 6 4 2
!$omp target update to(X(::2))
end
end
I actually wonder whether that restriction makes sense. For instance,
if an implementation handles the above correctly, then it likely handles
print *, X(ubound(X,1):lbound(X,1):-1)! prints: 1 2 3 4 5 6 7 8
!$omp target update to(X(::-1)) as well - and actually it very likely to
even regard this automatically as contiguous chunk of memory and just do
was instructed!
Regarding this part, I have now filed an OpenMP specification issue: #5106.
In any case, this is a hint that we better make 'stride' signed in the
struct passed to the runtime - just in case we need it in the future or
already now.
If unsupported in the runtime, the runtime error could also be moved there.
* * *
For handling unsupported stuff like iterator + array sections, if we add
a run-time check in the compiler: Unless we already marked that function
itself, I think
build_predict_expr (PRED_NORETURN, NOT_TAKEN) should be used to make
clear that this is the unlikely error case. In Fortran,
gfc_trans_runtime_check already takes care of this - when building the
check manually, it either the function decl needs to have noreturn or
the predict_expr is required. (Aside: setting TREE_THIS_VOLATILE on a
function decl seems to implies noreturn, which might be nonobvious but
is still consistent, cf.
https://gcc.gnu.org/onlinedocs/gccint/Function-Properties.html )
* * *
Thus, I was not planing to review this patch - but if you think that
this particular check is still useful, I might consider it.
Thanks,
Tobias
