This is an archive of the discontinued LLVM Phabricator instance.

[SYCL] Fix function pointer address space
ClosedPublic

Authored by eandrews on Oct 11 2021, 10:57 AM.

Details

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.

Diff Detail

Event Timeline

eandrews created this revision.Oct 11 2021, 10:57 AM
eandrews requested review of this revision.Oct 11 2021, 10:57 AM

need a test

eandrews updated this revision to Diff 379562.EditedOct 13 2021, 5:25 PM

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

bader accepted this revision.Oct 20 2021, 8:57 AM

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

This revision is now accepted and ready to land.Oct 20 2021, 8:57 AM
aaron.ballman added a subscriber: 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–637

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 added inline comments.Oct 21 2021, 8:50 AM
clang/lib/CodeGen/CodeGenTypes.cpp
636–637

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.

aaron.ballman added inline comments.Nov 4 2021, 7:24 AM
clang/lib/CodeGen/CodeGenTypes.cpp
636–637

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

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.

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 accepted this revision.Dec 1 2021, 1:18 AM

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 added inline comments.Dec 1 2021, 6:09 PM
clang/lib/CodeGen/CodeGenTypes.cpp
636–637

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

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 added inline comments.Jan 7 2022, 2:14 PM
clang/lib/CodeGen/CodeGenTypes.cpp
636–637

@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 added a comment.EditedJan 7 2022, 7:48 PM

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 updated this revision to Diff 398723.Jan 10 2022, 1:00 PM

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

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 added inline comments.Jan 10 2022, 1:58 PM
clang/lib/AST/ASTContext.cpp
11580

You can just do T->isFunctionType().

11583

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
747–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 added inline comments.Jan 10 2022, 4:38 PM
clang/lib/AST/ASTContext.cpp
11580

Thanks! I'll make the change

11583

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 updated this revision to Diff 398784.Jan 10 2022, 5:11 PM

Implement review comments - add a comment + remove unnecessary code

rjmccall added inline comments.Jan 10 2022, 5:55 PM
clang/lib/AST/ASTContext.cpp
11582

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.

11583

Ah, yes, I see. Alright.

eandrews marked 2 inline comments as done.Jan 11 2022, 6:57 AM
eandrews added inline comments.
clang/lib/AST/ASTContext.cpp
11582

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 updated this revision to Diff 399080.Jan 11 2022, 2:34 PM

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

eandrews marked 3 inline comments as done.Jan 11 2022, 2:45 PM
eandrews added inline comments.
clang/include/clang/Basic/TargetInfo.h
774

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 added inline comments.Jan 11 2022, 3:40 PM
clang/include/clang/Basic/TargetInfo.h
774

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 updated this revision to Diff 399484.Jan 12 2022, 3:13 PM

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 added inline comments.Jan 12 2022, 3:29 PM
clang/test/CodeGen/avr/functionptr-addrspace.c
4

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 accepted this revision.Jan 12 2022, 6:38 PM

LGTM, thanks!

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

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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 8:12 AM
arichardson added inline comments.
clang/lib/AST/ASTContext.cpp
11579

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

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.

Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2022, 2:15 AM