jdoerfert added a comment.
Conceptually, I am very confused but mostly because this patch conflates things.
I would suggest to split this part:
> This also refactors how the map clause is processed and separates the mlir
> and IRBuilder parts of the code similar to Clang.
it that is a better way of doing things.
Then, let's talks about use_device_ptr.
For one, I am not sure which test even shows the lowering.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1571
+ SmallDenseSet<Use *> Uses;
+ };
+
----------------
Everything in this file (should) come with documentation, including (most)
members.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4077
+ PointerType *i8PtrTy = Builder.getInt8PtrTy();
+ ArrayType *arrI8PtrTy = ArrayType::get(i8PtrTy, NumMapOperands);
+
----------------
Style, also elsewhere. I know it's annoying that MLIR broke with the LLVM
style, but that is what it is now.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4181-4191
+ for (auto &UseDeviceOp : UseDeviceInfos) {
+ auto *UseDeviceVal = UseDeviceOp.first;
+ auto &UseDeviceInfo = UseDeviceOp.second;
+ SmallDenseSet<Use *> ReplaceUses;
+ for (auto &Use : UseDeviceVal->uses()) {
+ if (UseDeviceInfo.Uses.find(&Use) == UseDeviceInfo.Uses.end())
+ ReplaceUses.insert(&Use);
----------------
TIFitis wrote:
> I couldn't come up with a better way for the code generated inside the
> `target data region` to use the new `AllocaPtrs` when accessing `map
> operands` marked with the `use_device_ptr` clause.
>
> Currently I am just comparing the `Users` list before and after generating
> the `data region` and replacing the new `uses`.
>
> Alternative method would be to use `mlir::LLVM::moduleTranslation.mapValue`
> method to temporarily remap the `mlir::Value` for the `use_device_ptr`
> operands to the `AllocaPtrs` `llvm::Value`. But `mapValue` doesn't allow
> remapping of values. @kiranchandramohan Is there a good reason to prevent
> remapping of values?
>
> Let me know if anyone has a better way of doing this :)
This is only a last resort. We should not generate stuff that is wrong and is
replaced. However, if we cannot find a better way we can talk about it.
Generally, I would have assumed that the value in the region in marked to be a
use_dev_ptr/addr and that we would generate code for that as soon as we
generate code for the value.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146557/new/
https://reviews.llvm.org/D146557
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
