[PATCH] D111566: [SYCL] Fix function pointer address space
Elizabeth Andrews via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 7 14:14:44 PST 2022
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
More information about the cfe-commits
mailing list