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

WaffleLapkin · 2022-05-11T14:06:57Z

This PR adds the following public API:

impl<T: ?Sized>*constT{fnmask(self,mask:usize) -> *constT;}impl<T: ?Sized>*mutT{fnmask(self,mask:usize) -> *constT;}// mod intrinsicsfnmask<T>(ptr:*constT,mask:usize) -> *constT

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

rust-highfive · 2022-05-11T14:07:01Z

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:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

WaffleLapkin · 2022-05-11T14:08:17Z

@rustbot label +T-libs-api -T-libs

RalfJung · 2022-05-11T14:12:02Z

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. :)

WaffleLapkin · 2022-05-11T14:22:08Z

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 · 2022-05-11T14:24:51Z

Can someone check that this is valid signature? llvm docs give the following signature:
declareptrty llvm.ptrmask(ptrty %ptr, intty %mask) readnone speculatable
And the rust intrinsic has the following signature:
fnptr_mask<T>(ptr:*constT,mask:usize) -> *constT;
Are all of these compatible with each other?

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

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

WaffleLapkin · 2022-05-11T14:26:18Z

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

bors · 2022-05-19T02:22:06Z

☔ The latest upstream changes (presumably #95643) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2022-06-20T04:06:22Z

this is still waiting on review

bors · 2022-07-27T15:39:25Z

☔ The latest upstream changes (presumably #99802) made this pull request unmergeable. Please resolve the merge conflicts.

apiraino · 2022-07-28T14:40:35Z

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

rustbot · 2022-08-05T14:47:22Z

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:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

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

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 · 2022-08-21T02:21:34Z

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 · 2022-08-21T18:51:22Z

@bors r+

bors · 2022-08-21T18:51:23Z

📌 Commit ca75312 has been approved by scottmcm

It is now in the queue for this repository.

RalfJung · 2022-08-27T14:42:38Z

(testing something)

@bors rollup=never

bors · 2022-08-28T01:34:50Z

⌛ Testing commit ca75312 with merge 1e978a3...

bors · 2022-08-28T04:16:23Z

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 1e978a3 to master...

rust-timer · 2022-08-28T19:18:25Z

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.

	mean¹	range	count²
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.

	mean¹	range	count²
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.

	mean¹	range	count²
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

the arithmetic mean of the percent change↩↩² ↩³
number of relevant changes↩↩² ↩³

Add pointer masking convenience functions This PR adds the following public API: ```rust 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

rust-highfive assigned joshtriplett May 11, 2022

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 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2022

rustbot added T-libs-api [DEPRECATED; DO NOT USE] and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 11, 2022

This comment has been minimized.
Sign in to view

WaffleLapkin commented May 11, 2022
View reviewed changes

WaffleLapkin force-pushed the ptr_mask branch from 8425129 to eedee27Compare May 11, 2022 14:59

bjorn3 reviewed May 11, 2022
View reviewed changes

Comment threadcompiler/rustc_codegen_cranelift/src/intrinsics/mod.rs Outdated

bjorn3 reviewed May 11, 2022
View reviewed changes

Comment threadcompiler/rustc_codegen_llvm/src/intrinsic.rs Outdated

scottmcm mentioned this pull request May 11, 2022
Wishlist: allow adding intrinsics in a way that doesn't break every backend #93145
Closed

bjorn3 reviewed May 12, 2022
View reviewed changes

Comment threadcompiler/rustc_codegen_gcc/src/intrinsic/mod.rs Outdated

WaffleLapkin force-pushed the ptr_mask branch from 958f952 to f83ec00Compare May 13, 2022 13:09

WaffleLapkin force-pushed the ptr_mask branch from f83ec00 to 8c191abCompare May 21, 2022 12:31

JohnTitor reviewed Jun 20, 2022
View reviewed changes

Comment threadlibrary/core/src/ptr/const_ptr.rs Outdated

WaffleLapkin mentioned this pull request Jun 20, 2022
Tracking Issue for pointer::mask#98290
Open
3 tasks

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022

WaffleLapkin force-pushed the ptr_mask branch from 2594976 to fee7ef6Compare August 5, 2022 14:47

WaffleLapkinand others added 4 commits August 21, 2022 05:27

fix cg cranelift
9c19120
Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>

Add codegen test for intinsics::ptr_mask
553f790

make ptr_mask codegen test more specific
55ba58c

Do not use void pointer for ptr_mask intrinsic
92b05db
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 force-pushed the ptr_mask branch from 5abe2dc to 92b05dbCompare August 21, 2022 02:17

scottmcm reviewed Aug 21, 2022
View reviewed changes

Comment threadsrc/test/codegen/intrinsics/mask.rs Outdated

This comment has been minimized.
Sign in to view

WaffleLapkin force-pushed the ptr_mask branch from 8f113b1 to 40670b6Compare August 21, 2022 02:43

This comment has been minimized.
Sign in to view

fix ptr_mask codegen test wrt llvm opaque pointers
ca75312

WaffleLapkin force-pushed the ptr_mask branch from 40670b6 to ca75312Compare August 21, 2022 03:04

WaffleLapkin requested a review from scottmcm August 21, 2022 06:09

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

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 28, 2022

bors merged commit 1e978a3 into rust-lang:masterAug 28, 2022

rustbot added this to the 1.65.0 milestone Aug 28, 2022

This was referenced Aug 28, 2022
Add round_ties_even to f32 and f64#95317
Merged
Add #[rustc_safe_intrinsic]#100719
Merged
Format rustc_codegen_gcc #101104
Closed

WaffleLapkin deleted the ptr_mask branch August 28, 2022 11:39

Uh oh!

Uh oh!

Conversation

WaffleLapkin commented May 11, 2022

Uh oh!

rust-highfive commented May 11, 2022

Uh oh!

WaffleLapkin commented May 11, 2022

Uh oh!

RalfJung commented May 11, 2022

Uh oh!

This comment has been minimized.

WaffleLapkin commented May 11, 2022

Uh oh!

WaffleLapkinMay 11, 2022

Choose a reason for hiding this comment

Uh oh!

scottmcmMay 11, 2022

Choose a reason for hiding this comment

Uh oh!

nikicMay 11, 2022

Choose a reason for hiding this comment

Uh oh!

WaffleLapkinMay 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented May 19, 2022

Uh oh!

JohnCSimon commented Jun 20, 2022

Uh oh!

Uh oh!

bors commented Jul 27, 2022

Uh oh!

apiraino commented Jul 28, 2022

Uh oh!

rustbot commented Aug 5, 2022

Uh oh!

WaffleLapkin commented Aug 21, 2022

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

scottmcm commented Aug 21, 2022

Uh oh!

bors commented Aug 21, 2022

Uh oh!

RalfJung commented Aug 27, 2022

Uh oh!

bors commented Aug 28, 2022

Uh oh!

bors commented Aug 28, 2022

Uh oh!

rust-timer commented Aug 28, 2022

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

17 participants