ABataev added inline comments.
================
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),
----------------
Do we really need to set `HasMapper` to `true` unconditionally here and in
other places?
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:841-842
+ /// Get the function for the specified user-defined mapper, if any.
+ virtual llvm::Function *
+ getUserDefinedMapperFunc(const OMPDeclareMapperDecl *D);
----------------
Is this required to make it `virtual`?
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:1556
return BasePointersArray && PointersArray && SizesArray &&
- MapTypesArray && NumberOfPtrs;
+ MapTypesArray && MappersArray && NumberOfPtrs;
}
----------------
`(!HasMapper || MappersArray)`?
================
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();
----------------
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.
================
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:
> 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.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7564-7573
void generateInfoForComponentList(
- OpenMPMapClauseKind MapType,
- ArrayRef<OpenMPMapModifierKind> MapModifiers,
+ OpenMPMapClauseKind MapType, ArrayRef<OpenMPMapModifierKind>
MapModifiers,
OMPClauseMappableExprCommon::MappableExprComponentListRef Components,
MapBaseValuesArrayTy &BasePointers, MapValuesArrayTy &Pointers,
MapValuesArrayTy &Sizes, MapFlagsArrayTy &Types,
- StructRangeInfoTy &PartialStruct, bool IsFirstComponentList,
- bool IsImplicit,
+ MapMappersArrayTy &Mappers, StructRangeInfoTy &PartialStruct,
+ bool IsFirstComponentList, bool IsImplicit,
----------------
lildmh wrote:
> ABataev wrote:
> > Too many params in the function, worth thinking about wrapping them in a
> > record.
> Maybe not in this patch?
It is better to make it in this patch or make a separate patch and commit it
before this one. We try to resolve this as soon as possible.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8821-8822
+
+ // No user-defined mapper for default mapping.
+ CurMappers.push_back(nullptr);
}
----------------
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.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8953-8955
+ llvm::Value *M = CGF.Builder.CreateConstInBoundsGEP2_32(
+ llvm::ArrayType::get(CGM.VoidPtrTy, Info.NumberOfPtrs),
+ Info.MappersArray, 0, I);
----------------
lildmh wrote:
> ABataev wrote:
> > Better to use `Builder.CreateConstArrayGEP`
> I saw all the other places are using `CreateConstInBoundsGEP2_32` to get
> sizes, types, etc. So I kept it consistent here. Maybe these modifications
> should be in a future patch?
Some of the modifications were made before we had these new functions. Now, we
have the new ones and if we can use it, better to switch to the new safer
versions.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8956-8957
+ Info.MappersArray, 0, I);
+ M = CGF.Builder.CreatePointerBitCastOrAddrSpaceCast(
+ M, MFunc->getType()->getPointerTo(/*AddrSpace=*/0));
+ Address MAddr(M, Ctx.getTypeAlignInChars(Ctx.VoidPtrTy));
----------------
lildmh wrote:
> ABataev wrote:
> > Easier cast function to the `voidptr`, I think
> Again, this is to keep it consistent with previous code above?
No need to rely on the old code if can make something cleaner and better. Cast
Mfunc to `voidptr` in the `if` statement and no need to make some extra casts
later.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8991-8994
+ MappersArrayArg = CGF.Builder.CreateConstInBoundsGEP2_32(
+ llvm::ArrayType::get(CGM.VoidPtrTy, Info.NumberOfPtrs),
+ Info.MappersArray,
+ /*Idx0=*/0, /*Idx1=*/0);
----------------
lildmh wrote:
> ABataev wrote:
> > Hmm, just cast of the `Info.MappersArray` to the correct type if all
> > indices are `0`?
> Again, this is to keep consistent with previous code?
Same, use the best code if we can use it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67833/new/
https://reviews.llvm.org/D67833
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
