eandrews added inline comments.
================ Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:636-638 + unsigned AS = PointeeType->isFunctionTy() + ? getDataLayout().getProgramAddressSpace() + : Context.getTargetAddressSpace(ETy); ---------------- rjmccall wrote: > aaron.ballman wrote: > > eandrews wrote: > > > aaron.ballman wrote: > > > > The review summary says that this is a fix for SYCL, but the fix itself > > > > happens for all targets, not just SYCL. If that's intentional, are we > > > > sure it's correct? > > > Yes this affects all targets. To be honest, I'm not sure if this change > > > is correct for CUDA/openCL, etc. My first patch (which I didn't upload) > > > restricted the change to SYCL. However, I saw the same thing was done in > > > a generic manner for function pointers - > > > https://github.com/llvm/llvm-project/commit/57fd86de879cf2b4c7001b6d0a09df60877ce24d, > > > and so followed the same logic. I'm hoping reviewers more familiar with > > > address spaces can help here. > > @Anastasia -- can you comment as OpenCL code owner? > I think the more systematic fix is probably for `getTargetAddressSpace` to > check for function types and return the program address space, yeah. @rjmccall @dylanmckay I took a look into this today. Moving the code to `getTargetAddressSpace ` causes a few openCL tests to fail because address space changes for block pointers. ``` case Type::BlockPointer: { const QualType FTy = cast<BlockPointerType>(Ty)->getPointeeType(); llvm::Type *PointeeType = CGM.getLangOpts().OpenCL ? CGM.getGenericBlockLiteralType() : ConvertTypeForMem(FTy); unsigned AS = Context.getTargetAddressSpace(FTy); ResultType = llvm::PointerType::get(PointeeType, AS); break; } ``` Here PointeeType (FTy) is a function type and so Context.getTargetAddressSpace(FTy) will return ProgramAddressSpace after my change. IR Change - ``` void foo(){ int (^ block_B)(void) = ^{ return i; }; block_B(); } OLDIR %block_B = alloca %struct.__opencl_block_literal_generic addrspace(4)*, align 4 --- NEWIR %block_B = alloca %struct.__opencl_block_literal_generic*, align 4 ``` Would you know if this is correct behavior for block pointers? Or is the existing behavior correct here? I apologize for the delay in responding to your review. I did not get a chance to work on this before I left for vacation last month. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111566/new/ https://reviews.llvm.org/D111566 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
