[PATCH] D111566: [SYCL] Fix function pointer address space

arichardson added inline comments. Herald added a project: All. Comment at: clang/lib/AST/ASTContext.cpp:11579 +unsigned ASTContext::getTargetAddressSpace(QualType T) const { + return T->isFunctionType() ? getTargetInfo().getProgramAddressSpace() + :

[PATCH] D111566: [SYCL] Fix function pointer address space

This revision was automatically updated to reflect the committed changes. Closed by commit rG4eaf5846d0e7: [clang] Fix function pointer address space (authored by eandrews). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall accepted this revision. rjmccall added a comment. LGTM, thanks! Comment at: clang/test/CodeGen/avr/functionptr-addrspace.c:3 + +int main() { + int (*p)(); eandrews wrote: > When writing the test I noticed an existing crash in the compiler. I am not

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews added inline comments. Comment at: clang/test/CodeGen/avr/functionptr-addrspace.c:3 + +int main() { + int (*p)(); When writing the test I noticed an existing crash in the compiler. I am not sure if I am doing something wrong but if you try to assign

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews updated this revision to Diff 399484. eandrews added a comment. Herald added a subscriber: Jim. Implemented review comments - Remove unnecessary set method and set default value during construction instead - Changed default address space for function pointer for avr target to 1.

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:763 + /// Set the address space for functions for the given target. + virtual void setProgramAddressSpace() { ProgramAddrSpace = 0; } + eandrews wrote: > I'm not entirely sure

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews marked 3 inline comments as done. eandrews added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:763 + /// Set the address space for functions for the given target. + virtual void setProgramAddressSpace() { ProgramAddrSpace = 0; } +

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews updated this revision to Diff 399080. eandrews added a comment. Add program address space to TargetInfo. Targets with non-default address space for functions should explicitly set value. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111566/new/

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews marked 2 inline comments as done. eandrews added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:11496 + .getProgramAddressSpace() + : getTargetAddressSpace(T.getQualifiers()); +} rjmccall wrote: > Oh, I'm sorry I

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:11496 + .getProgramAddressSpace() + : getTargetAddressSpace(T.getQualifiers()); +} Oh, I'm sorry I missed this. Parsing the data layout string into an

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews updated this revision to Diff 398784. eandrews added a comment. Implement review comments - add a comment + remove unnecessary code CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111566/new/ https://reviews.llvm.org/D111566 Files: clang/include/clang/AST/ASTContext.h

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:11494 + const Type *TypePtr = T.getTypePtr(); + return TypePtr->isFunctionType() + ? llvm::DataLayout(getTargetInfo().getDataLayoutString()) rjmccall wrote: > You can just do

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:11494 + const Type *TypePtr = T.getTypePtr(); + return TypePtr->isFunctionType() + ? llvm::DataLayout(getTargetInfo().getDataLayoutString()) You can just do

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews added a comment. In D111566#3228949 , @rjmccall wrote: > Block pointers are actually data pointers and should stay in the generic > address space if they don't have an address space qualifier. That might > require new logic. Thanks! I kept

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews updated this revision to Diff 398723. eandrews added a comment. Implemented review comment to move logic into function (`getTargetAddressSpace`) as opposed to call site. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111566/new/ https://reviews.llvm.org/D111566 Files:

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall added a comment. Block pointers are actually data pointers and should stay in the generic address space. I... don't honestly know why they get lowered as function pointers; they probably shouldn't be. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111566/new/

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews added inline comments. Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:636-638 +unsigned AS = PointeeType->isFunctionTy() + ? getDataLayout().getProgramAddressSpace() + : Context.getTargetAddressSpace(ETy);

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews added a comment. Thanks for the reviews @dylanmckay and @rjmccall ! I agree that moving the logic for functions pointers to `getTargetAddressSpace` makes sense. However, I'm not sure what the consequences are, since that increases the impact of this change quite a bit. I'm not sure

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:636-638 +unsigned AS = PointeeType->isFunctionTy() + ? getDataLayout().getProgramAddressSpace() + : Context.getTargetAddressSpace(ETy);

[PATCH] D111566: [SYCL] Fix function pointer address space

dylanmckay accepted this revision. dylanmckay added a comment. By the way, as this has already been approved by one, and you rightly applied the "speak now or forever hold your peace" principle re. OpenCL, and this clearly works better from my point of view than the old code, I wouldn't want

[PATCH] D111566: [SYCL] Fix function pointer address space

dylanmckay added a comment. This will product correct code from an AVR perspective where (before this patch it would have failed codegen). It is consistent with how we construct function pointers in LLVM core as well. I'm not very familiar with Clang internals, one thing that stick out to me:

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews added a comment. ping * 3 Since this patch has been accepted by one code owner, and has been under review for over a month, I plan to submit it by the end of the week. If anyone has feedback/concerns, please comment on the review. CHANGES SINCE LAST ACTION

[PATCH] D111566: [SYCL] Fix function pointer address space

aaron.ballman added inline comments. Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:636-638 +unsigned AS = PointeeType->isFunctionTy() + ? getDataLayout().getProgramAddressSpace() + : Context.getTargetAddressSpace(ETy);

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews added a comment. ping 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

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews added inline comments. Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:636-638 +unsigned AS = PointeeType->isFunctionTy() + ? getDataLayout().getProgramAddressSpace() + : Context.getTargetAddressSpace(ETy);

[PATCH] D111566: [SYCL] Fix function pointer address space

aaron.ballman added reviewers: rjmccall, erichkeane, Anastasia. aaron.ballman added a comment. Adding a few more reviewers with more familiarity with codegen and address spaces to make sure we've not missed something here outside of SYCL. Comment at:

[PATCH] D111566: [SYCL] Fix function pointer address space

bader accepted this revision. bader added a comment. This revision is now accepted and ready to land. @vlastik, your commit fixes function pointers on AVR - https://github.com/llvm/llvm-project/commit/57fd86de879cf2b4c7001b6d0a09df60877ce24d. I suppose this change is required for fixing lvalue

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews added a comment. ping 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

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews updated this revision to Diff 379562. eandrews added a comment. Added a test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111566/new/ https://reviews.llvm.org/D111566 Files: clang/lib/CodeGen/CodeGenTypes.cpp clang/test/CodeGenSYCL/functionptr-addressspace.cpp Index:

[PATCH] D111566: [SYCL] Fix function pointer address space

yaxunl added a comment. need a test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111566/new/ https://reviews.llvm.org/D111566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D111566: [SYCL] Fix function pointer address space

eandrews created this revision. eandrews added reviewers: vlastik, dylanmckay, bader. Herald added subscribers: Naghasan, Anastasia, ebevhan, yaxunl. eandrews requested review of this revision. Functions pointers should be created with program address space. This patch fixes a crash on lvalue