[PATCH] D67833: [OpenMP 5.0] Codegen support to pass user-defined mapper functions to runtime
Alexey Bataev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 14 09:48:02 PST 2019
ABataev added a comment.
I would suggest to split this patch into 2 part: the first one is NFC and just makes the ompiler to use the new interfaces and the second with the new functionality for mappers.
================
Comment at: include/clang/AST/OpenMPClause.h:4298
+ /// use_device_ptr and is_device_ptr.
+ bool hasMapper;
+
----------------
Not in Camel format
================
Comment at: include/clang/AST/OpenMPClause.h:4543
/// of the class.
- ArrayRef<Expr *> getUDMapperRefs() const {
- return llvm::makeArrayRef<Expr *>(
- static_cast<T *>(this)->template getTrailingObjects<Expr *>() +
+ ArrayRef<const Expr *> getUDMapperRefs() const {
+ assert(hasMapper &&
----------------
Why changed to `const Expr *` here? I believe, `ArrayRef` makes it constant automatically.
================
Comment at: include/clang/AST/OpenMPClause.h:4596
+ // Whether this clause is possible to have user-defined mappers associated.
+ bool hasMapper;
+
----------------
Not in Camel format
================
Comment at: include/clang/AST/OpenMPClause.h:4599
+ // The user-defined mapper associated with the current declaration.
+ ArrayRef<const Expr *>::iterator MapperCur;
+
----------------
No need for const in `ArrayRef`
================
Comment at: include/clang/AST/OpenMPClause.h:4621
ArrayRef<unsigned> CumulativeListSizes,
- MappableExprComponentListRef Components)
+ MappableExprComponentListRef Components, bool hasMapper,
+ ArrayRef<const Expr *> Mappers)
----------------
Not in Camel format
================
Comment at: include/clang/AST/OpenMPClause.h:4743
+ getComponentsRef(), hasMapper,
+ hasMapper ? getUDMapperRefs() : ArrayRef<const Expr *>());
}
----------------
`ArrayRef<const Expr *>()`->`llvm::None`?
================
Comment at: include/clang/AST/OpenMPClause.h:4750
+ getComponentsRef().end()),
+ hasMapper, ArrayRef<const Expr *>());
}
----------------
`ArrayRef<const Expr *>()`->`llvm::None`
================
Comment at: include/clang/AST/OpenMPClause.h:4763
+ getComponentListSizesRef(), getComponentsRef(), hasMapper,
+ hasMapper ? getUDMapperRefs() : ArrayRef<const Expr *>());
}
----------------
Same here
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:742
// Call to void __tgt_target_data_end(int64_t device_id, int32_t arg_num,
- // void** args_base, void **args, size_t *arg_sizes, int64_t *arg_types);
+ // void** args_base, void **args, int64_t *arg_sizes, int64_t *arg_types);
OMPRTL__tgt_target_data_end,
----------------
Better to fix it in a separate patch
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:755-794
+ // Call to int32_t __tgt_target_mapper(int64_t device_id, void *host_ptr,
+ // int32_t arg_num, void** args_base, void **args, int64_t *arg_sizes, int64_t
+ // *arg_types, void **arg_mappers);
+ OMPRTL__tgt_target_mapper,
+ // Call to int32_t __tgt_target_nowait_mapper(int64_t device_id, void
+ // *host_ptr, int32_t arg_num, void** args_base, void **args, int64_t
+ // *arg_sizes, int64_t *arg_types, void **arg_mappers);
----------------
I believe these new functions replace completely the old one, so old functions must be removed
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:2746
+ break;
+ }
case OMPRTL__tgt_mapper_num_components: {
----------------
Same here, remove processing for old functions.
================
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();
----------------
CamelCase
================
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:
> 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.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7411
+ if (hasMapper)
+ return CGF.Builder.getInt64(1);
+ else
----------------
Why do we return `1` here?
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7412
+ return CGF.Builder.getInt64(1);
+ else
+ return CGF.getTypeSize(BaseTy);
----------------
No need for `else` here
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7431-7434
+ return CGF.Builder.getInt64(1);
+ else
+ return ElemSize;
+ }
----------------
Same here
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7445
+ return LengthVal;
+ else
+ return CGF.Builder.CreateNUWMul(LengthVal, ElemSize);
----------------
No need for `else` here
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7463-7472
+ if (hasMapper)
+ return CGF.Builder.CreateExactUDiv(LengthVal, ElemSize);
+ else
+ return LengthVal;
}
- return CGF.getTypeSize(ExprTy);
+ // In case that a user-defined mapper is attached, its size is the
+ // number of array elements instead of the number of total bytes.
----------------
Same here
================
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,
----------------
Too many params in the function, worth thinking about wrapping them in a record.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7940-7943
+ if (hasMapper)
+ Mappers.push_back(Mapper);
+ else
+ Mappers.push_back(nullptr);
----------------
`Mappers.push_back(HasMappers?Mapper:nullptr);`
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8716-8718
+ if (!std::get<0>(L))
continue;
+ const auto *VD = dyn_cast<VarDecl>(std::get<0>(L));
----------------
`const auto *VD = dyn_cast_or_null<VarDecl>(std::get<0>(L));`
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8821-8822
+
+ // No user-defined mapper for default mapping.
+ CurMappers.push_back(nullptr);
}
----------------
In general, there should not be branch for default mapping during the codegen, the implicit map clauses must be generated 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);
----------------
Better to use `Builder.CreateConstArrayGEP`
================
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));
----------------
Easier cast function to the `voidptr`, I think
================
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);
----------------
Hmm, just cast of the `Info.MappersArray` to the correct type if all indices are `0`?
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8995-8996
+ /*Idx0=*/0, /*Idx1=*/0);
+ } else
+ MappersArrayArg = llvm::ConstantPointerNull::get(CGM.VoidPtrPtrTy);
} else {
----------------
If `then` sub-statement in braces, the `else` substatement also must be in braces
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9359
+ assert(MapperFunc && "Expect a valid mapper function is available.");
+ MapperCGF.Builder.CreateCall(MapperFunc, OffloadingArgs);
+ } else {
----------------
Better to use `MapperCGF.EmitNounwindRuntimeCall`.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:1533
+ /// Indicate whether any user-defined mapper exists.
+ bool hasMapper = false;
/// The total number of pointers passed to the runtime library.
----------------
Camel case
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:1549
MapTypesArray = nullptr;
+ MappersArray = nullptr;
NumberOfPtrs = 0u;
----------------
The flag `HasMappers` also must be cleared, no?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67833/new/
https://reviews.llvm.org/D67833
More information about the cfe-commits
mailing list