sfantao added inline comments. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7787-7789 @@ -7786,5 +7786,5 @@ "bit fields cannot be used to specify storage in a map clause">; -def err_omp_array_section_in_rightmost_expression : Error< - "array section can only be associated with the rightmost variable in a map clause expression">; +def err_cannot_prove_omp_array_section_specifies_contiguous_storage : Error< + "can't prove employed array section specifies contiguous storage">; def err_omp_union_type_not_allowed : Error< "mapped storage cannot be derived from a union">; ---------------- ABataev wrote: > 'can't prove' again is not good for an error. Still think this must be a > warning or you don't need to diagnose anything if you can't make a decision. > In this case, you must consider specified array section as a contiguous. Now I am only emitting an error for the cases that can be proved wrong. For those that are inconclusive I don't issue any warning. The message also changed to reflect that.
================ Comment at: lib/Sema/SemaOpenMP.cpp:9016-9023 @@ +9015,10 @@ + + // If this is an array subscript, it refers to the whole size if the size of + // the dimension is constant and equals 1. Also, an array section assumes the + // format of an array subscript if no colon is used. + if (isa<ArraySubscriptExpr>(E) || (OASE && OASE->getColonLoc().isInvalid())) { + if (auto *ATy = dyn_cast<ConstantArrayType>(BaseQTy.getTypePtr())) + return ATy->getSize().getSExtValue() == 1; + return false; + } + ---------------- ABataev wrote: > ABataev wrote: > > I agree with all your examples except for this one > > ``` > > #pragma omp target map(a[:][arg:]) > > ``` > > For me, this is valid if you can't prove it is non-contiguous. You should > > consider this as a possibly contiguous. If it is not contiguous, it is > > user's problem. But if he needs to use such form of expression, I don't see > > why we should not allow him to do this. > I don't think this code > ``` > const int n = 0; > arr[n:] > ``` > will work in C. Try to add a test for C, not C++. In C 'const' does not mean > you can evaluate the value of the variable during compilation. In the new diff I am allowing this given that the ranges check is inconclusive. At runtime the behavior will be the same as map(a[:]), so the runtime map will contain the holes. This falls into those cases that is users' fault. ================ Comment at: lib/Sema/SemaOpenMP.cpp:9016-9023 @@ +9015,10 @@ + + // If this is an array subscript, it refers to the whole size if the size of + // the dimension is constant and equals 1. Also, an array section assumes the + // format of an array subscript if no colon is used. + if (isa<ArraySubscriptExpr>(E) || (OASE && OASE->getColonLoc().isInvalid())) { + if (auto *ATy = dyn_cast<ConstantArrayType>(BaseQTy.getTypePtr())) + return ATy->getSize().getSExtValue() == 1; + return false; + } + ---------------- sfantao wrote: > ABataev wrote: > > ABataev wrote: > > > I agree with all your examples except for this one > > > ``` > > > #pragma omp target map(a[:][arg:]) > > > ``` > > > For me, this is valid if you can't prove it is non-contiguous. You should > > > consider this as a possibly contiguous. If it is not contiguous, it is > > > user's problem. But if he needs to use such form of expression, I don't > > > see why we should not allow him to do this. > > I don't think this code > > ``` > > const int n = 0; > > arr[n:] > > ``` > > will work in C. Try to add a test for C, not C++. In C 'const' does not > > mean you can evaluate the value of the variable during compilation. > In the new diff I am allowing this given that the ranges check is > inconclusive. At runtime the behavior will be the same as map(a[:]), so the > runtime map will contain the holes. This falls into those cases that is > users' fault. So if you have: ``` const int n = 0; arr[:][n:] ``` This is not a probably anymore because the check in the second dimension will be inconclusive, so it will be allowed even if n can't be proved to be zero. I'm adding a test anyways. http://reviews.llvm.org/D17547 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits