Skip to content

Prepare GEP building for opaque pointers #87695

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
merged 4 commits into from
Aug 4, 2021
Merged

Prepare GEP building for opaque pointers #87695

merged 4 commits into from
Aug 4, 2021

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Aug 2, 2021 •
edited
Loading

No description provided.

@rust-highfive
Copy link
Contributor

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2021
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 2, 2021

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned matthewjasper Aug 2, 2021
nagisa
nagisa reviewed Aug 4, 2021
View reviewed changes
compiler/rustc_codegen_ssa/src/mir/place.rs
@@ -103,12 +103,13 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
if offset == a.value.size(bx.cx()).align_to(b.value.align(bx.cx()).abi) =>
{
// Offset matches second field.
bx.struct_gep(self.llval, 1)
let ty = bx.backend_type(self.layout);
bx.struct_gep(ty, self.llval, 1)
}
Abi::Scalar(_) | Abi::ScalarPair(..) | Abi::Vector { .. } if field.is_zst() => {
// ZST fields are not included in Scalar, ScalarPair, and Vector layouts, so manually offset the pointer.
let byte_ptr = bx.pointercast(self.llval, bx.cx().type_i8p());
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to pointercast this self.llval anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When an explicit pointee type is provided it must be equal to the pointer element type, so this will have to wait until migration to opaque pointers is complete:

https://github.com/rust-lang/llvm-project/blob/260e0f8682098faab68af9c608534756ad378365/llvm/include/llvm/IR/Instructions.h#L956-L958

nagisa
nagisa reviewed Aug 4, 2021
View reviewed changes
compiler/rustc_codegen_ssa/src/mir/place.rs
@@ -185,7 +189,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {

// Cast and adjust pointer.
let byte_ptr = bx.pointercast(self.llval, bx.cx().type_i8p());
Copy link
Member

Choose a reason for hiding this comment

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

Similarly here.

@nagisa
Copy link
Member

nagisa commented Aug 4, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 4, 2021
@bors
Copy link
Collaborator

bors commented Aug 4, 2021

⌛ Trying commit 6aafd29e3d4588c170d88707da1f90308843d33f with merge 414880045b5e5aa73962982e332456daa6a1162a...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Aug 4, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2021
tmiasko added 4 commits August 4, 2021 15:51
@tmiasko
Prepare struct_gep for opaque pointers
838042a 
Imlement struct_gep using LLVMBuildStructGEP2 which takes an explicit
type argument instead of deriving it from a pointer type.
@tmiasko
Prepare gep for opaque pointers
4013e09 
Implement gep using LLVMBuildGEP2 which takes an explicit type argument
instead of deriving it from a pointer type.
@tmiasko
Prepare inbounds_gep for opaque pointers
77e5e17 
Implement inbounds_gep using LLVMBuildInBoundsGEP2 which takes an
explicit type argument instead of deriving it from a pointer type.
@tmiasko
Replace LLVMConstInBoundsGEP with LLVMConstInBoundsGEP2*
8e0df32 
A custom reimplementation of LLVMConstInBoundsGEP2 is used, since the
LLVM contains a declaration of LLVMConstInBoundsGEP2 but not the
implementation.
@tmiasko tmiasko force-pushed the gep-opaque-pointers branch from 6aafd29 to 8e0df32 Compare August 4, 2021 13:52
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 4, 2021

@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 4, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Collaborator

bors commented Aug 4, 2021

⌛ Trying commit 8e0df32 with merge 83871193ae29d0a5032d80fd9102b851d62bbb4f...

@bors
Copy link
Collaborator

bors commented Aug 4, 2021

☀️ Try build successful - checks-actions
Build commit: 83871193ae29d0a5032d80fd9102b851d62bbb4f (83871193ae29d0a5032d80fd9102b851d62bbb4f)

@rust-timer
Copy link
Collaborator

Queued 83871193ae29d0a5032d80fd9102b851d62bbb4f with parent 87d713f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (83871193ae29d0a5032d80fd9102b851d62bbb4f): comparison url.

Summary: This benchmark run did not return any significant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 4, 2021
@nagisa
Copy link
Member

nagisa commented Aug 4, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 4, 2021

📌 Commit 8e0df32 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2021
@bors
Copy link
Collaborator

bors commented Aug 4, 2021

⌛ Testing commit 8e0df32 with merge d54fbb9...

@bors
Copy link
Collaborator

bors commented Aug 4, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing d54fbb9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 4, 2021
@bors bors merged commit d54fbb9 into rust-lang:master Aug 4, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 4, 2021
@tmiasko tmiasko deleted the gep-opaque-pointers branch August 4, 2021 21:21
@bors bors mentioned this pull request Aug 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nagisa nagisa nagisa left review comments

Assignees

@nagisa nagisa

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.
8 participants
@tmiasko @rust-highfive @nagisa @rust-timer @bors @rust-log-analyzer @matthewjasper @rustbot