[PATCH] D67833: [OpenMP 5.0] Codegen support to pass user-defined mapper functions to runtime

Wed Dec 18 11:45:38 PST 2019

ABataev added inline comments.

================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8981-8982
+  // Convert the size in bytes into the number of array elements.
+  Size = MapperCGF.Builder.CreateExactUDiv(
+      Size, MapperCGF.Builder.getInt64(ElementSize.getQuantity()));
   llvm::Value *PtrBegin = MapperCGF.Builder.CreateBitCast(
----------------
lildmh wrote:
> ABataev wrote:
> > lildmh wrote:
> > > ABataev wrote:
> > > > lildmh wrote:
> > > > > ABataev wrote:
> > > > > > ABataev wrote:
> > > > > > > lildmh wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > lildmh wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > So, we're still going to use number of elements for mappers? And pass it in the same parameter that in other cases is used as size in bytes? If so, point to it explicitly in the review for the runtime part so all are informed about it.
> > > > > > > > > > From interface, the mapper function uses size in bytes now. Inside, it needs number of elements to iterate through all elements. This has no impact on the runtime part, since it looks like normal mapping from the interface. All conversion happens inside the mapper function which is completely generated by the compiler.
> > > > > > > > > Ok. Then why do we need to convert size in bytes to number of elements here?
> > > > > > > > This is used to 1) see if we are going to map an array of elements with mapper, and 2) iterate all to map them individually.
> > > > > > > Could you point where we have this kind of analysis here? Because I don't see anything affected by this change in the patch.
> > > > > > Is this a bug fix in the previous implementation?
> > > > > The previous implementation assumes the size is the number of elements, and it works correctly under that assumption. Since we change the meaning of size here, I add this line of code so the previous implementation can work correctly in the new assumption that the size is the size in bytes.
> > > > Ah, got it. Then, in general, looks good. Please, split the patches in to, possibly, 2 NFC (one with using of the new functions + another one for aggregating too many params into records) + another one with the new functionality.
> > > Okay, will do that. What do you think need to be done for the runtime patch D68100?
> > Address comments and rebase, I think
> The only left comment is to split it into 2 patches, I think.
Do it.

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

https://reviews.llvm.org/D67833