Add pointer masking convenience functions by WaffleLapkin · Pull Request #96946 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation62 Commits10 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

WaffleLapkin

This PR adds the following public API:

impl<T: ?Sized> *const T { fn mask(self, mask: usize) -> *const T; }

impl<T: ?Sized> *mut T { fn mask(self, mask: usize) -> *const T; }

// mod intrinsics fn mask(ptr: *const T, mask: usize) -> *const T

This is equivalent to ptr.map_addr(|a| a & mask) but also uses a cool llvm intrinsic.

Proposed in #95643 (comment)

cc @Gankra @scottmcm @RalfJung

r? rust-lang/libs-api

@rustbot rustbot added T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

May 11, 2022

@rust-highfive

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

@WaffleLapkin

@rustbot rustbot added T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.

and removed T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

May 11, 2022

@RalfJung

Makes sense! Should this be added to the strict provenance feature gate?

Also, if/when this lands it'd be great if you could also implement the new intrinsic for Miri. :)

@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin

Should this be added to the strict provenance feature gate?

I don't really have an opinion on this. While this is mostly useful to mask pointers without touching provenance it may also be used as a shorter version of ptr as uszie & mask (vs ptr.mask(mask)).

Also, if/when this lands it'd be great if you could also implement the new intrinsic for Miri. :)

Sure, I'll take a look into that!

WaffleLapkin

@@ -887,6 +888,9 @@ impl<'ll> CodegenCx<'ll, '_> {
ifn!("llvm.dbg.declare", fn(self.type_metadata(), self.type_metadata()) -> void);
ifn!("llvm.dbg.value", fn(self.type_metadata(), t_i64, self.type_metadata()) -> void);
}
ifn!("llvm.ptrmask", fn(voidp, t_isize) -> voidp);

Choose a reason for hiding this comment

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

Can someone check that this is valid signature? llvm docs give the following signature:

declare ptrty llvm.ptrmask(ptrty %ptr, intty %mask) readnone speculatable

And the rust intrinsic has the following signature:

fn ptr_mask(ptr: *const T, mask: usize) -> *const T;

Are all of these compatible with each other?

Choose a reason for hiding this comment

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

The LLVM docs confuse me, here. The semantic section says it's possible for the bitwidth of the mask to be different from the pointer size of the target, but that sounds to me like it'd be an overloaded intrinsic, and thus named something like llvm.ptrmask.i64 the same way llvm.cttz.* and friends work. But the name isn't documented the way those ones are...

Choose a reason for hiding this comment

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

Yes, the name would be llvm.ptrmask.p0i8.i64 etc.

WaffleLapkin

Comment on lines +1200 to +1316

/// Note that, unlike most intrinsics, this is safe to call;
/// it does not require an `unsafe` block.
/// Therefore, implementations must not require the user to uphold
/// any safety invariants.

Choose a reason for hiding this comment

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

If I understood llvm docs correctly, ptrmask is equivalent to just GEP, not GEP in bounds, so this is safe.

bjorn3

bjorn3

bjorn3

@bors

@JohnCSimon

this is still waiting on review

JohnTitor

@bors

@apiraino

I think this is now waiting for a feedback from T-libs-api cc @joshtriplett (who was autoassigned)

@rustbot

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

@WaffleLapkin @bjorn3

Co-authored-by: bjorn3 17426603+bjorn3@users.noreply.github.com

@WaffleLapkin

@WaffleLapkin

@WaffleLapkin

I couldn't find where exactly it's documented, but apperantly pointers to void type are invalid in llvm - void is only allowed as a return type of functions.

@WaffleLapkin

So, it seems like in the ~1000 commits since I've made this branch llvm was updated bringing opaque pointers with it 😄

cc @nikic, actually -- should it be @llvm.ptrmask.{{ptr|p0isVoid}}.[[WORD]]( to make sure not to fail in the opaque pointers future?

It seems it should have been {{p0|p0i8}} (at least with the new checkout I get p0 and before that I was getting p0i8)

scottmcm

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin

@scottmcm

@bors

📌 Commit ca75312 has been approved by scottmcm

It is now in the queue for this repository.

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

labels

Aug 21, 2022

@RalfJung

(testing something)

@bors rollup=never

@bors

@bors

This was referenced

Aug 28, 2022

@rust-timer

Finished benchmarking commit (1e978a3): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌ (primary) - - 0
Regressions ❌ (secondary) - - 0
Improvements ✅ (primary) - - 0
Improvements ✅ (secondary) -1.1% [-1.1%, -1.1%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌ (primary) - - 0
Regressions ❌ (secondary) 3.2% [3.2%, 3.2%] 1
Improvements ✅ (primary) - - 0
Improvements ✅ (secondary) -4.9% [-9.4%, -1.6%] 5
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌ (primary) 3.5% [3.5%, 3.5%] 1
Regressions ❌ (secondary) 2.3% [2.3%, 2.3%] 1
Improvements ✅ (primary) - - 0
Improvements ✅ (secondary) - - 0
All ❌✅ (primary) 3.5% [3.5%, 3.5%] 1

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3
  2. number of relevant changes ↩ ↩2 ↩3

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request

Oct 23, 2022

@bors

Add pointer masking convenience functions

This PR adds the following public API:

impl<T: ?Sized> *const T {
    fn mask(self, mask: usize) -> *const T;
}

impl<T: ?Sized> *mut T {
    fn mask(self, mask: usize) -> *const T;
}

// mod intrinsics
fn mask<T>(ptr: *const T, mask: usize) -> *const T

This is equivalent to ptr.map_addr(|a| a & mask) but also uses a cool llvm intrinsic.

Proposed in rust-lang#95643 (comment)

cc @Gankra @scottmcm @RalfJung

r? rust-lang/libs-api

antoyo pushed a commit to antoyo/rust that referenced this pull request

Jun 19, 2023

@bors

Add pointer masking convenience functions

This PR adds the following public API:

impl<T: ?Sized> *const T {
    fn mask(self, mask: usize) -> *const T;
}

impl<T: ?Sized> *mut T {
    fn mask(self, mask: usize) -> *const T;
}

// mod intrinsics
fn mask<T>(ptr: *const T, mask: usize) -> *const T

This is equivalent to ptr.map_addr(|a| a & mask) but also uses a cool llvm intrinsic.

Proposed in rust-lang#95643 (comment)

cc @Gankra @scottmcm @RalfJung

r? rust-lang/libs-api

Reviewers

@nikic nikic nikic left review comments

@RalfJung RalfJung RalfJung left review comments

@antoyo antoyo antoyo left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@JohnTitor JohnTitor JohnTitor left review comments

@scottmcm scottmcm Awaiting requested review from scottmcm

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.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.