[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis marked 2 inline comments as done.
TIFitis added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4064-4077
+    Value *BaseGEPPtr = Builder.CreateInBoundsGEP(
+        ArrI8PtrTy, MapperAllocas.ArgsBase,
+        {Builder.getInt32(0), Builder.getInt32(Index)});
+    Value *BaseCastPtr = Builder.CreateBitCast(
+        BaseGEPPtr, MapOpPtrBase->getType()->getPointerTo());
+    Builder.CreateStore(MapOpPtrBase, BaseCastPtr);
+    MapInfo.BasePtr = BaseGEPPtr;
----------------
I think these should just use the opaque ptr type instead of the ArrI8PtrTy it is currently using, and we can also get rid of the BitCast instructions.

However doing so would break the tests in OpenMPIRBuilderTest.cpp as that unit test still uses typed pointers.

AFAIK the consensus is to completely get rid of typed pointers in llvm 17. Should I updated this code to use opaque pointers and let the test fail until it gets converted to opaque as well?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4261
+  bool Temp;
+  return Val->getPointerDereferenceableBytes(M.getDataLayout(), Temp, Temp);
 }
----------------
jdoerfert wrote:
> TIFitis wrote:
> > `getPointerDereferenceableBytes` doesn't fully support `Arguments` and mostly returns 0.
> > 
> > I am looking at adding support for arguments in a separate patch, in the meantime the tests added in this patch fail because of incorrect @.offload_sizes.
> That function is not the right one for code generation. The sizes need to come from the type + data layout, and potentially the expression used in the map.
Thanks for letting me know.

Just to clarify, do you mean it should come from the MLIR::Type?

The llvm::Type doesn't let me get much information because of Opaque pointers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557