lildmh marked 5 inline comments as done.
lildmh added a comment.
Alexey, thanks for the review
================
Comment at: clang/include/clang/AST/OpenMPClause.h:4918
const OMPMappableExprListSizeTy &Sizes)
- : OMPMappableExprListClause(OMPC_map, Locs, Sizes, &MapperQualifierLoc,
- &MapperIdInfo),
+ : OMPMappableExprListClause(OMPC_map, Locs, Sizes, /*HasMapper=*/true,
+ &MapperQualifierLoc, &MapperIdInfo),
----------------
ABataev wrote:
> Do we really need to set `HasMapper` to `true` unconditionally here and in
> other places?
For `map`, `to`, and `from` clauses, they can have mappers, so it's set to
`true`. For `is_device_ptr` and `use_device_ptr`, it's set to `false`.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7388
- llvm::Value *getExprTypeSize(const Expr *E) const {
+ llvm::Value *getExprTypeSize(const Expr *E, bool hasMapper) const {
QualType ExprTy = E->getType().getCanonicalType();
----------------
ABataev wrote:
> ABataev wrote:
> > lildmh wrote:
> > > ABataev wrote:
> > > > ABataev wrote:
> > > > > CamelCase
> > > > Seems to me, with mapper you're changing the contract of this function.
> > > > It is supposed to return the size in bytes, with Mapper it returns just
> > > > number of elements. Bad decision. Better to convert size in bytes to
> > > > number of elements in place where you need it.
> > > Yes. The reason why I did this is I found this is the single place that I
> > > can consider all situations. Otherwise, I need to divide the size by the
> > > element size in other places. Since this function has discussed all
> > > situations for the element size, I will need to duplicate the work
> > > elsewhere if I don't do it here. The latter solution is not good for code
> > > mountainous I think.
> > Changing the original function contract is also not the best solution.
> > Better to introduce the new function. If some of the code can be reused,
> > need to factor out the common code and reuse it in the old function and the
> > new one.
> Also, additional question. Does it mean that we pass to the runtime functions
> different values in different situations: in one case we pass size in bytes,
> with mappers - number of elements? If so, it is a very bad idea caused by bad
> design. We need to pass either size in bytes, or number of elements in this
> argument.
Yes, the current design is like this. Let me see how to make it better
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8821-8822
+
+ // No user-defined mapper for default mapping.
+ CurMappers.push_back(nullptr);
}
----------------
ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > In general, there should not be branch for default mapping during the
> > > codegen, the implicit map clauses must be generated in sema.
> > Not sure where this code will be used. I guess it's still correct to add a
> > null to the mappers here?
> I mean, that in general, it would be good to see here some kind of an
> assertion. Almost all situations must be resolved in Sema.
Okay, I guess it's not the work of this patch
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67833/new/
https://reviews.llvm.org/D67833
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
