Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Opaque Pointer] Fix bad code within INTEL_SYCL_OPAQUEPOINTER_READY #10870

Merged
merged 5 commits into from
Aug 22, 2023
Merged

[Opaque Pointer] Fix bad code within INTEL_SYCL_OPAQUEPOINTER_READY #10870

merged 5 commits into from
Aug 22, 2023

Conversation

jsji
Copy link
Contributor

@jsji jsji commented Aug 17, 2023

This is in preparation for setting -DINTEL_SYCL_OPAQUEPOINTER_READY=1

  • Fix bad guard for INTEL_SYCL_OPAQUEPOINTER_READY
  • Keep the addressspace cast for opaque pointer
  • Fix opaque pointer guard for getVirtualFunctionPointer

@jsji jsji requested review from a team as code owners August 17, 2023 23:51
@jsji jsji requested review from bader and cdai2 August 17, 2023 23:52
@jsji jsji self-assigned this Aug 17, 2023
@jsji
Copy link
Contributor Author

jsji commented Aug 17, 2023

@cdai2 Please have a look at ce2ff56 , as this is corresponding changes in opaque pointer patch for OpenCL.

@jsji jsji closed this Aug 18, 2023
@jsji jsji reopened this Aug 18, 2023
@jsji
Copy link
Contributor Author

jsji commented Aug 18, 2023

Ping @intel/dpcpp-cfe-reviewers @intel/dpcpp-tools-reviewers @cdai2

The changes also tested with -DINTEL_SYCL_OPAQUEPOINTER_READY=1 in #10888

Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, why tools team is summoned as code owners here, files are quite out of scope of our expertise
yet the changes look legit

@jsji
Copy link
Contributor Author

jsji commented Aug 21, 2023

Thanks @MrSidims !

not sure, why tools team is summoned as code owners here, files are quite out of scope of our expertise yet the changes look legit

We have intel/dpcpp-tools-reviewers as code owner of ALL llvm changes. Is this intended? If not, we probably should update it.

# DPC++ tools
llvm/ @intel/dpcpp-tools-reviewers

clang/lib/CodeGen/ABIInfoImpl.cpp Outdated Show resolved Hide resolved
clang/lib/CodeGen/ItaniumCXXABI.cpp Show resolved Hide resolved
Co-authored-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

uint64_t VTableIndex = CGM.getItaniumVTableContext().getMethodVTableIndex(GD);
llvm::Value *VFunc;
if (CGF.ShouldEmitVTableTypeCheckedLoad(MethodDecl->getParent())) {
VFunc = CGF.EmitVTableTypeCheckedLoad(
#ifdef INTEL_SYCL_OPAQUEPOINTER_READY
MethodDecl->getParent(), VTable, PtrTy,
#else
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like to me that we can avoid this #ifdef if we do the rename of TyPtr to PtrTy.
Other than this, we have to keep all the other #ifdef anyway, due to call to ->getPoitnerTo() , BitCast etc.

@jsji
Copy link
Contributor Author

jsji commented Aug 22, 2023

@intel/llvm-gatekeepers Can we get this merged? Thanks!

@steffenlarsen steffenlarsen merged commit f727cee into intel:sycl Aug 22, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants