lildmh added a comment.
I'll split the patch into 2 later
================
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:
> > 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.
================
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,
----------------
ABataev wrote:
> Too many params in the function, worth thinking about wrapping them in a
> record.
Maybe not in this patch?
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8821-8822
+
+ // No user-defined mapper for default mapping.
+ CurMappers.push_back(nullptr);
}
----------------
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?
================
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);
----------------
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?
================
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));
----------------
ABataev wrote:
> Easier cast function to the `voidptr`, I think
Again, this is to keep it consistent with previous code above?
================
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);
----------------
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?
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
