[SYCL] Fix function pointer address space

[eandrews]

Summary

Functions pointers should be created with program address space. This patch fixes a crash on lvalue reference to function pointer (in device code) when using oneAPI DPC++ compiler.

[yaxunl]

need a test

[eandrews]

Thanks for taking a look @yaxunl! I added a test.

[eandrews]

ping

[bader]

@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 references to function pointers on AVR as well. Right?

[aaron.ballman]

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.

clang/lib/CodeGen/CodeGenTypes.cpp

636–639

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?

[eandrews]

clang/lib/CodeGen/CodeGenTypes.cpp

636–639

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.

[eandrews]

ping

[aaron.ballman]

clang/lib/CodeGen/CodeGenTypes.cpp

636–639

@Anastasia -- can you comment as OpenCL code owner?

[eandrews]

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.

[dylanmckay]

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:

The logic instead belongs directly inside the existing getTargetAddressSpace method?

Perhaps the new if (is function) { set address space to the value from the data layout} logic belongs directly inside ASTContext::getTargetAddressSpace or one of its descendants in the call tree. This would obviously increase the scope/possible impact of the change further. The reason I suspect it belongs in there is that in theory, if this is the correct way to get the address space for a QualType (which may be a function) in this instance, then I feel that same logic would hold for any other QualType that may be a function pointer that has getTargetAddressSpace() called on it because I don't see anything special or unique about this existing call to getTargetAddressSpace versus any other existing call to it inside clang. If you implement it at that lower level inside getTargetAddressSpace, your conditional would be something like QualType.getTypePtr()->isFunctionType() etc.

This patch fixes one callsite of getTargetAddressSpace but there are several other existing callsites remaining which if called with a function, they *would not* return the appropriate address space.

If someone more familar with clang than me disagrees please chime in

[dylanmckay]

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 to prevent you from committing this. I have no objections to merging this in its current state. If you do merge it as-is though it would be nice if a follow up PR moves the logic into getTargetAddressSpace() where I think it better belongs

[rjmccall]

clang/lib/CodeGen/CodeGenTypes.cpp

636–639

I think the more systematic fix is probably for getTargetAddressSpace to check for function types and return the program address space, yeah.

[eandrews]

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 if I will have the time to deal with any issues that arise before I go on vacation for Christmas. I'll take a quick look sometime this week, and hopefully its a simple enough change. If not, I can do it in a follow-up PR as suggested above, unless someone objects.

[eandrews]

clang/lib/CodeGen/CodeGenTypes.cpp

636–639

@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.

[rjmccall]

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.

[eandrews]

Implemented review comment to move logic into function (getTargetAddressSpace) as opposed to call site.

[eandrews]

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 the existing behavior for block pointers by passing qualifiers to getTargetAddressSpace.

[rjmccall]

clang/lib/AST/ASTContext.cpp

11494 ↗

(On Diff #398723)

You can just do T->isFunctionType().

11497 ↗

(On Diff #398723)

If a function type has an address space qualifier, we should prefer that, right? Or is that impossible by construction?

clang/lib/CodeGen/CodeGenTypes.cpp

753

Please add a comment here explaining that it's important to not just use FTy because in the absence of an explicit address space we need to get the default address space for a data pointer, not a function.

[eandrews]

clang/lib/AST/ASTContext.cpp

11494 ↗

(On Diff #398723)

Thanks! I'll make the change

11497 ↗

(On Diff #398723)

I thought you could only use address space qualifiers for variables. I am not sure though.

This patch retains existing behavior for pointers. The existing code (deleted in CodeGenTypes.cpp in his patch) doesn't take qualifiers into consideration.

[eandrews]

Implement review comments - add a comment + remove unnecessary code

[rjmccall]

clang/lib/AST/ASTContext.cpp

11496 ↗

(On Diff #398784)

Oh, I'm sorry I missed this. Parsing the data layout string into an llvm::DataLayout is definitely not an okay thing to be doing here. The IRGen version of this had a cached DataLayout object which it queried, which was okay, but this function is used too tightly to be doing that much redundant work.

We could just cache a DataLayout in the clang::TargetInfo, but I think we've been trying to avoid that as a layering violation. Instead, TargetInfo should just have a getProgramAddressSpace method or something like that, and the targets with non-default address spaces for code should set that manually.

11497 ↗

(On Diff #398723)

Ah, yes, I see. Alright.

[eandrews]

clang/lib/AST/ASTContext.cpp

11496 ↗

(On Diff #398784)

Instead, TargetInfo should just have a getProgramAddressSpace method or something like that, and the targets with non-default address spaces for code should set that manually.

Ok. Thanks for review! I'll make this change

[eandrews]

Add program address space to TargetInfo. Targets with non-default address space for functions should explicitly set value.

[eandrews]

clang/include/clang/Basic/TargetInfo.h

763 ↗

(On Diff #399080)

I'm not entirely sure about setting the default value to zero here. I set it to 0 here based on default value for ProgramAddrSpace in DataLayout class (https://llvm.org/doxygen/DataLayout_8cpp_source.html#l00186)

Targets with non-zero value should override this function. I do not know what these targets are (if any). I ran check-llvm and nothing failed.

[rjmccall]

clang/include/clang/Basic/TargetInfo.h

763 ↗

(On Diff #399080)

I don't think you need this method; targets that want to change the program address space can just set it during construction, since it's a protected field. I know there are some existing places that use this virtual set pattern, but there doesn't seem to be a good reason for it.

Looks like AVR sets the program address space to 1. (I did a regexp search for /["-]P[0-9]/ in lib/Basic/.)

[eandrews]

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.

[eandrews]

clang/test/CodeGen/avr/functionptr-addrspace.c

3 ↗

(On Diff #399484)

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 to the pointer, you will hit the following assert - https://llvm.org/doxygen/Constants_8cpp_source.html#l02280

int f() { return 0; }

int main() {
  int (*p)() = f;
  return 0;
}

I briefly debugged this and I think the crash is because in GetAddrOfFunction we call ConvertType which returns type without the address space here - https://clang.llvm.org/doxygen/CodeGenTypes_8cpp_source.html#l00419.

I didn't fix this or debug this further since it is outside the scope of this patch but I can look into it as a followup PR

[rjmccall]

LGTM, thanks!

clang/test/CodeGen/avr/functionptr-addrspace.c

3 ↗

(On Diff #399484)

Okay. AVR may be a constrained enough environment that they don't actually run into this sort of thing, but yeah, it'd be great to chase these bugs down.

[arichardson]

clang/lib/AST/ASTContext.cpp

11579 ↗

(On Diff #399680)

Can we add a DataLayout aware function to CodeGen instead? That would avoid the need for getProgramAddressSpace() in TargetInfo?

clang/lib/Basic/TargetInfo.cpp

153 ↗

(On Diff #399680)

A bit late to this review (only just noticed it while merging) - but I don't like that we end up duplicating even more DataLayout information here - while it only affects AVR upstream, downstream CHERI and Morello have to duplicate this to Arm/RISC-V/MIPS as well now.

clang/lib/CodeGen/CodeGenTypes.cpp

case Type::Complex: {
     ResultType = llvm::StructType::get(EltTy, EltTy);
     break;
   }
   case Type::LValueReference:
   case Type::RValueReference: {
     const ReferenceType *RTy = cast<ReferenceType>(Ty);
     QualType ETy = RTy->getPointeeType();
     llvm::Type *PointeeType = ConvertTypeForMem(ETy);
-    unsigned AS = Context.getTargetAddressSpace(ETy);
+    unsigned AS = PointeeType->isFunctionTy()
+                      ? getDataLayout().getProgramAddressSpace()
+                      : Context.getTargetAddressSpace(ETy);
     ResultType = llvm::PointerType::get(PointeeType, AS);
     break;
   }
   case Type::Pointer: {
     const PointerType *PTy = cast<PointerType>(Ty);
     QualType ETy = PTy->getPointeeType();
     llvm::Type *PointeeType = ConvertTypeForMem(ETy);
     if (PointeeType->isVoidTy())
       PointeeType = llvm::Type::getInt8Ty(getLLVMContext());
case Type::Enum: {
     break;
   }
 
   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;
   }
 
   case Type::MemberPointer: {
     auto *MPTy = cast<MemberPointerType>(Ty);
     if (!getCXXABI().isMemberPointerConvertible(MPTy)) {
       RecordsWithOpaqueMemberPointers.insert(MPTy->getClass());