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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
