Skip to content

[NFC][Opaque pointer][LLVM] Remove code in ifndef INTEL_SYCL_OPAQUEPOINTER_READY #11063

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.

Sign up for GitHub

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

Jump to bottom
Merged
aelovikov-intel merged 2 commits into from
Sep 8, 2023
Merged

[NFC][Opaque pointer][LLVM] Remove code in ifndef INTEL_SYCL_OPAQUEPOINTER_READY #11063

aelovikov-intel merged 2 commits into from
Sep 8, 2023

Conversation

@jsji
Copy link
Contributor

@jsji jsji commented Sep 5, 2023 •
edited
Loading

  • Remove code guarded in ifndef INTEL_SYCL_OPAQUEPOINTER_READY

This should be NFC, as INTEL_SYCL_OPAQUEPOINTER_READY is always 1 since #10888 .

@jsji jsji self-assigned this Sep 5, 2023
@jsji
Copy link
Contributor Author

jsji commented Sep 6, 2023

@aelovikov-intel Looks like we need to manually clean up some temp file in 'amdgpu-3' runner.

fatal: update_ref failed for ref 'HEAD': cannot lock ref 'HEAD': Unable to create '/__w/repo_cache/intel/llvm/.git/HEAD.lock': File exists.

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Sep 6, 2023

@aelovikov-intel Looks like we need to manually clean up some temp file in 'amdgpu-3' runner.

fatal: update_ref failed for ref 'HEAD': cannot lock ref 'HEAD': Unable to create '/__w/repo_cache/intel/llvm/.git/HEAD.lock': File exists.

Done, I think. I've removed repo_cache/intel/llvm/ and restarted the task. cached_checkout action recreates the cache when it's missing.

@jsji jsji marked this pull request as ready for review September 6, 2023 13:40
@jsji jsji requested a review from a team as a code owner September 6, 2023 13:40
@jsji
Copy link
Contributor Author

jsji commented Sep 6, 2023

Failures in AMD/HIP is CI issue .

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Sep 6, 2023

Failures in AMD/HIP is CI issue .

There isn't enough information to merge this without full testing passing.

@jsji
Copy link
Contributor Author

jsji commented Sep 6, 2023

Failures in AMD/HIP is CI issue .

There isn't enough information to merge this without full testing passing.

Sure. Trying to re-run AMD/HIP dozen times without success, trying again.

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Sep 6, 2023

Sure. Trying to re-run AMD/HIP dozen times without success, trying again.

I've just initiated reboot of amd-02 runner.

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Sep 6, 2023

Also, if INTEL_SYCL_OPAQUEPOINTER_READY is universally true, then it should be made clear in the description to highlight the "NFC" nature of this change.

Why aren't we changing any CMakeLists.txt files here?

@jsji
Copy link
Contributor Author

jsji commented Sep 6, 2023 •
edited
Loading

Also, if INTEL_SYCL_OPAQUEPOINTER_READY is universally true, then it should be made clear in the description to highlight the "NFC" nature of this change.

Sure, added.

Why aren't we changing any CMakeLists.txt files here?

Good question, I leave them intentionally to next PR once all these PRs (#11063, #11062, #11059) are merged -- just in case that we remove the definition in CMakeListx.txt and we fall back to some old code path unintentionally.

@jsji jsji changed the title [Opaque pointer][LLVM] Remove code in ifndef INTEL_SYCL_OPAQUEPOINTER_READY [NFC][Opaque pointer][LLVM] Remove code in ifndef INTEL_SYCL_OPAQUEPOINTER_READY Sep 6, 2023
@jsji jsji requested a review from bader September 6, 2023 18:56
@jsji
Copy link
Contributor Author

jsji commented Sep 6, 2023

Ping @intel/dpcpp-tools-reviewers . Thanks.

asudarsa
asudarsa reviewed Sep 7, 2023
View reviewed changes
llvm/lib/IR/Core.cpp
return unwrap(Ty)->isOpaquePointerTy();
#endif // INTEL_SYCL_OPAQUEPOINTER_READY
}
LLVMBool LLVMPointerTypeIsOpaque(LLVMTypeRef Ty) { return true; }
Copy link
Contributor

@asudarsa asudarsa Sep 7, 2023

Choose a reason for hiding this comment

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

I think we can remove this function and its uses? But that needs to happen in llorg though.

Thanks

Copy link
Contributor Author

@jsji jsji Sep 8, 2023

Choose a reason for hiding this comment

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

Yes, we will sync with llorg on this. We will start to remove usage of it in our own code first, after removing code guarded in INTEL_SYCL_OPAQUEPOINTER_READY. After that, we will consider removing it if community also remove it.

asudarsa
asudarsa approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

Changes look good on the whole.

Thanks for putting this up.

@jsji
Copy link
Contributor Author

jsji commented Sep 8, 2023

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

@aelovikov-intel aelovikov-intel merged commit 14f5c18 into intel:sycl Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asudarsa asudarsa asudarsa approved these changes

@bader bader Awaiting requested review from bader

Assignees

@jsji jsji

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jsji @aelovikov-intel @asudarsa