Skip to content

[SYCL] Reland community changes to support RawAddress #13358

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 1 commit into from
Apr 25, 2024

Conversation

premanandrao
Copy link
Contributor

As it was a complex set of changes
llvm/llvm-project@84780af
was not applied during normal pulldown. This PR relands that commit as well as a couple of
additional fixes needed.

@premanandrao
Copy link
Contributor Author

@intel/dpcpp-cfe-reviewers, can I please get a review on this? You only need to review the minor changes in my second commit. The first commit is a community cherry-pick.

Copy link
Contributor

@smanna12 smanna12 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@elizabethandrews
Copy link
Contributor

Can you explain why the additional fixes were needed?

@premanandrao
Copy link
Contributor Author

Can you explain why the additional fixes were needed?

Not sure what you are asking, but the getPointer member function was removed from the Address class.

@elizabethandrews
Copy link
Contributor

Can you explain why the additional fixes were needed?

Not sure what you are asking, but the getPointer member function was removed from the Address class.

I did not realize the code you modified was not present in community. I was wondering why the original PR did not make these changes.

@premanandrao
Copy link
Contributor Author

@intel/llvm-gatekeepers, can this be merged please? The test failure about code formatting points to community changes not the additional changes that I made.

@ldrumm
Copy link
Contributor

ldrumm commented Apr 12, 2024

Can you please manually squash the commits so author attribution is correct?

image

Since the bulk of the work here is by @ahatanak

…needed to authenticate signed pointers (#86923)

To authenticate pointers, CodeGen needs access to the key and
discriminators that were used to sign the pointer. That information is
sometimes known from the context, but not always, which is why `Address`
needs to hold that information.

This patch adds methods and data members to `Address`, which will be
needed in subsequent patches to authenticate signed pointers, and uses
the newly added methods throughout CodeGen. Although this patch isn't
strictly NFC as it causes CodeGen to use different code paths in some
cases (e.g., `mergeAddressesInConditionalExpr`), it doesn't cause any
changes in functionality as it doesn't add any information needed for
authentication.

In addition to the changes mentioned above, this patch introduces class
`RawAddress`, which contains a pointer that we know is unsigned, and
adds several new functions for creating `Address` and `LValue` objects.

This reapplies d9a685a, which was
reverted because it broke ubsan bots. There seems to be a bug in
coroutine code-gen, which is causing EmitTypeCheck to use the wrong
alignment. For now, pass alignment zero to EmitTypeCheck so that it can
compute the correct alignment based on the passed type (see function
EmitCXXMemberOrOperatorMemberCallExpr).

Additional fixes needed
@premanandrao
Copy link
Contributor Author

Can you please manually squash the commits so author attribution is correct?

Since the bulk of the work here is by @ahatanak

Thank you for pointing this out. Should be fixed now. (Unless I messed it up.)

@bader
Copy link
Contributor

bader commented Apr 12, 2024

Can you please manually squash the commits so author attribution is correct?

image

Since the bulk of the work here is by @ahatanak

4563116 - the author attribution was correct before the manual squash. As far as I can tell it's preserved by GitHub squash and merge, so manual squash is not needed. @ldrumm, did I miss anything?

@premanandrao
Copy link
Contributor Author

@intel/llvm-gatekeepers, will it help if I commit this in as two separate patches? One: that is exclusively the community changes and two: my changes? For a brief period in between, our builds will break, but my changes will fix that. That will keep the authorship entirely separate.

(There is further downstream work that I need to do after this is merged here. Hence the time sensitivity.)

@ldrumm
Copy link
Contributor

ldrumm commented Apr 15, 2024

No.Even with your manual squash (which I thought would work) there's apparently no way to preserve correct authorship, nor the original commit message. We need an admin to merge this manually rather than github do it.

Our policy is broken in the face of cherry picks or similar

@ldrumm
Copy link
Contributor

ldrumm commented Apr 15, 2024

@stdale-intel Can you merge this patch manually with the complete, correctly attributed 02b4109, please?

@steffenlarsen steffenlarsen merged commit 9191ceb into intel:sycl Apr 25, 2024
10 of 21 checks passed
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.

8 participants