Skip to content

[SYCL][NVPTX] Emit 'grid_constant' annotations for by-val kernel params #14332

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 8 commits into from
Jul 10, 2024
Merged

[SYCL][NVPTX] Emit 'grid_constant' annotations for by-val kernel params #14332

merged 8 commits into from
Jul 10, 2024

Conversation

frasercrmck
Copy link
Contributor

@frasercrmck frasercrmck commented Jun 27, 2024 •
edited
Loading

Also fix up the DeadArgumentElimination passes to correctly preserve the
annotations; when removing arguments from functions, dead parameters
need pruned and alive ones may need their values shifted down by the
number of dead arguments that came before them.

@akshayrdeodhar @frasercrmck
[NVPTX] Basic support for "grid_constant" (#96125)
922c2d5 
- Adds a helper function for checking whether an argument is a
[grid_constant](https://docs.nvidia.com/cuda/nvvm-ir-spec/index.html#supported-properties).
- Adds support for cvta.param using changes from
llvm/llvm-project#95289
- Supports escaped grid_constant pointers conservatively, by casting all
uses to the generic address space with cvta.param.
@frasercrmck frasercrmck requested review from a team as code owners June 27, 2024 16:41
@frasercrmck
[SYCL][NVPTX] Emit 'grid_constant' NVVM annotations for by-val kernel…
4a9216f 
… params

Also fix up the DeadArgumentElimination passes to correctly preserve the
annotations; when removing arguments from functions, dead parameters
need pruned and alive ones may need their values shifted down by the
number of dead arguments that came before them.
@bader
Copy link
Contributor

bader commented Jun 27, 2024

NOTE TO REVIEWERS this PR includes a cherry-pick of a recent upstream commit which has not yet been pulled down. Without it, the NVPTX backend crashes when processing this new type of annotation. If this is merged before this cherry-pick is pulled down, it would be best to leave it unsquashed. Otherwise we can drop it from the PR.

The commits will be squashed. If you don't want to wait for a pulldown, I recommend opening a separate PR with cherry-pick only.

Naghasan
Naghasan reviewed Jun 27, 2024
View reviewed changes
@ldrumm
Copy link
Contributor

ldrumm commented Jul 1, 2024

NOTE TO REVIEWERS this PR includes a cherry-pick of a recent upstream commit which has not yet been pulled down. Without it, the NVPTX backend crashes when processing this new type of annotation. If this is merged before this cherry-pick is pulled down, it would be best to leave it unsquashed. Otherwise we can drop it from the PR.

The commits will be squashed. If you don't want to wait for a pulldown, I recommend opening a separate PR with cherry-pick only.

Seems like a massive waste of time to work around a bad policy.

@ldrumm
Copy link
Contributor

ldrumm commented Jul 1, 2024

Oh, and the cherry picked commit is not by the present author, so unless that author joined this project and created that merge request, the authorship will be incorrect.

I think it's best we maintain correct attribution of copyrights

@Naghasan Naghasan mentioned this pull request Jul 3, 2024
Merged
@elizabethandrews
Copy link
Contributor

@premanandrao can you take a look at this? I am off till Wednesday.

@frasercrmck
Copy link
Contributor Author

Oh, and the cherry picked commit is not by the present author, so unless that author joined this project and created that merge request, the authorship will be incorrect.

I think it's best we maintain correct attribution of copyrights

Yes it doesn't seem like the current system can correctly support cherry-picks in PRs without manual intervention.

Either way, the commit we depend on has been merged as part of the pulldown, so this PR is unblocked in that regard.

premanandrao
premanandrao approved these changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

FE changes LGTM.

@frasercrmck
Copy link
Contributor Author

@intel/llvm-gatekeepers this looks good to merge, thank you!

@sommerlukas sommerlukas merged commit d77a348 into intel:sycl Jul 10, 2024
14 checks passed
@frasercrmck frasercrmck deleted the sycl-grid-constant branch July 10, 2024 08:39
@frasercrmck frasercrmck mentioned this pull request Jul 10, 2024
Closed
@Naghasan Naghasan mentioned this pull request Dec 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@Naghasan Naghasan Naghasan approved these changes

@premanandrao premanandrao premanandrao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@frasercrmck @bader @ldrumm @elizabethandrews @AlexeySachkov @Naghasan @premanandrao @sommerlukas @akshayrdeodhar