Skip to content

Backport rust-lang/rust#134117 and allow only <*T>::offset-like "GEPs". #327

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

Merged
merged 6 commits into from
Jul 9, 2025

Conversation

eddyb
Copy link
Collaborator

@eddyb eddyb commented Jul 7, 2025

(Each commit should be reviewed separately)


LLVM's GetElementPointer (akin to SPIR-V OpAccessChain/OpPtrAccessChain) was, for a long time, the main way to interact with pointers, as it could perform, in one go, all of
e.g.: &(*(p: *Foo).add(i)).outer_field3[j].0.inner_field1[k].leaf_field2, even today:

%out = getelementptr Foo, ptr %p, i64 %i, i32 3, i64 %j, i32 0, i32 1, i64 %k, i32 2

continues to work, even in the "untyped pointer" world (i.e. %p having type ptr instead of Foo*).

However, both in LLVM itself, and in rustc_codegen_llvm, its patterns have been shrinking with time:

And they're generally converging on <*T>::offset-like pointer arithmetic, where T is:

  • in the general case: the element type of a [T; N] or [T] type
    • e.g. <[T]>::get_unchecked(xs, i) is &*(xs as *const [T] as *const T).add(i)
  • (common) special-case: i8/u8 for arbitrary byte offsetting
    • often used with a constant offset, for e.g. forming pointers to struct fields
    • in theory all Ts could go through byte offsetts by multiplying w/ the size of T, aka "stride", but having that multiplication in the IR is not (yet) on the table
      (even SPIR-T qptr uses a special "strided offset", even if it erases the specific type T)

Instead of keeping the increasingly-gnarly maybe_inbounds_gep (see also e.g. #233 - a valiant effort at reconciling several of its conflicting needs), this PR moves the core logic to a new method,
named ptr_offset_strided (suffixed _strided to hopefully avoid confusion with the byte-oriented ptradd).

By only handling (p: *T).offset(i), no more, no less (i.e. no intra-T offsetting), a lot of the decision logic/correctness subtleties/etc., vanish away, fixing edge cases like #233 (comment) without breaking the issue-46 test (which #233 itself first fixed).

Sadly I wasn't able to keep a lot of the comments/trace!s added in #233, but I tried to have a bare minimum, combined with the continued existence of #[instrument] on/trace! in, several helper methods.


EDIT: on top of "future rustup PRs", the SPIR-V validation error described in issue #323 appears to disappear on top of this PR (as reported in #323 (comment)) - so it's possible none of this code was ever bug-free.

@eddyb eddyb marked this pull request as draft July 7, 2025 11:33
@Firestar99
Copy link
Member

Firestar99 commented Jul 7, 2025

Builds with my project 🎉

Copy link
Collaborator

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Haven't fully ramped up on this code again, but I can't see anything wrong with it so far.

@eddyb eddyb force-pushed the bye-gep branch 2 times, most recently from f6ab6ae to 48d94f6 Compare July 9, 2025 13:10
@eddyb eddyb marked this pull request as ready for review July 9, 2025 13:11
@eddyb eddyb enabled auto-merge July 9, 2025 13:11
@eddyb eddyb added this pull request to the merge queue Jul 9, 2025
Merged via the queue into Rust-GPU:main with commit ace9310 Jul 9, 2025
13 checks passed
@eddyb eddyb deleted the bye-gep branch July 9, 2025 14:01
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.

3 participants