[PATCH] D74144: [OPENMP50]Add basic support for array-shaping operation.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev marked 4 inline comments as done.
ABataev added a comment.

In D74144#1941971 <https://reviews.llvm.org/D74144#1941971>, @rjmccall wrote:

> Okay, so these array-shaping expressions are only allowed as operands to specific OpenMP directives?  That's a plausible interpretation of "The shape-operator can appear only in clauses where it is explicitly allowed" from the spec <https://www.openmp.org/spec-html/5.0/openmpsu20.html>.  If it were a more general l-value expression, we could handle that just fine by building the type using `ConstantArrayType`/`VariableArrayType` as appropriate; but if the language intentionally restricts it, the placeholder approach seems fine.


Hi John, thanks for the review!
Yes, this is not a general form of the expression, it is allowed only in a couple of clauses.
We cannot build an array type since this expression does not represent a supported array type. Sizes may be non-constant anywhere in the operation, variable array type does not support it. It requires extending of the variable array types but actually it does not worth a try, since this type is not needed at all.



================
Comment at: clang/lib/Sema/SemaExpr.cpp:4748
+       Base->isInstantiationDependent() ||
+       Base->containsUnexpandedParameterPack()))
+    return OMPArrayShapingExpr::Create(Context, Context.DependentTy, Base,
----------------
rjmccall wrote:
> Just check `isTypeDependent()`, none of these other conditions should interfere with type-checking.
Ok, will do.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4751
+                                       LParenLoc, RParenLoc, Dims, Brackets);
+  if (!BaseTy->isAnyPointerType())
+    return ExprError(Diag(Base->getExprLoc(),
----------------
rjmccall wrote:
> I think you should perform DefaultFunctionArrayLvalueConversion here so that e.g. arrays will decay to pointers, you load pointers from l-values, and so on.  If you do so, it'll handle placeholders for you.
> 
> Do you really want to allow this to operate on non-C pointer types?
1. Standard clearly states that the type of the base expression must be a pointer. I don't think that we should perform implicit type casting here, like decay to pointers, etc.
2. It is just a simple form of checking that this is a pointer type. Since this expression is not allowed in other languages (and I filter it out at the parsing stage), I think it is ok to use the generic form of type checking.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4758
+  for (Expr *Dim : Dims) {
+    if (Dim->getType()->isNonOverloadPlaceholderType()) {
+      ExprResult Result = CheckPlaceholderExpr(Dim);
----------------
rjmccall wrote:
> I think overload placeholders need to be resolved here, too.  You may have copied this code from some different place that has the ability to resolve overloads later, but in this case that's not true.
No, YouCompleteMe suggested the wrong function and I just missed it. Will fix it, thanks!


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4771
+    }
+    if (!Dim->isValueDependent() && !Dim->isTypeDependent()) {
+      ExprResult Result =
----------------
rjmccall wrote:
> You don't really care about value-dependence here, just type-dependence.  You can check value-dependence before doing the constant-evaluation check below.
Yes, just a double check to be realy-really sure :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74144/new/

https://reviews.llvm.org/D74144