Skip to content
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

[PowerPC] [MMA] Add lxvp/stxvp and pair assemble/disassemble builtins with _mma_ prefix #49503

Closed
ahsansaghir opened this issue Apr 29, 2021 · 15 comments
Assignees
Labels
bugzilla Issues migrated from bugzilla clang:headers Headers provided by Clang, e.g. for intrinsics

Comments

@ahsansaghir
Copy link
Contributor

Bugzilla Link 50159
Version trunk
OS Linux
Blocks #48661
CC @DougGregor,@slacka,@nemanjai,@zygoloid,@tstellar
Fixed by commit(s) 25bbff6

Extended Description

Vector pair intrinsics and builtins were renamed in
https://reviews.llvm.org/D91974 to replace the mma prefix by vsx.
However, some projects used the mma version, so this patch adds
these intrinsics to provide compatibility:

__builtin_mma_lxvp -> __builtin_vsx_lxvp
__builtin_mma_stxvp -> __builtin_vsx_stxvp
__builtin_mma_assemble_pair -> __builtin_vsx_assemble_pair
__builtin_mma_disassemble_pair -> __builtin_vsx_disassemble_pair

@ahsansaghir
Copy link
Contributor Author

assigned to @hfinkel

@tstellar
Copy link
Collaborator

Vector pair intrinsics and builtins were renamed in
https://reviews.llvm.org/D91974 to replace the mma prefix by vsx.
However, some projects used the mma version, so this patch adds
these intrinsics to provide compatibility:

__builtin_mma_lxvp -> __builtin_vsx_lxvp
__builtin_mma_stxvp -> __builtin_vsx_stxvp
__builtin_mma_assemble_pair -> __builtin_vsx_assemble_pair
__builtin_mma_disassemble_pair -> __builtin_vsx_disassemble_pair

Which patch adds the intrinsics?

@ahsansaghir
Copy link
Contributor Author

Sorry, missed the link for the patch:
Differential Revision: https://reviews.llvm.org/D100482

@ahsansaghir
Copy link
Contributor Author

Also, there is an update after the review: the patch adds the mma version of the built-ins as aliases to the existing vsx intrinsics.

@ahsansaghir
Copy link
Contributor Author

@slacka
Copy link
Mannequin

slacka mannequin commented May 7, 2021

Reopening for 12.x backport

@tstellar
Copy link
Collaborator

The fix does not apply cleanly, could someone backport this and push a branch to their local github fork?

@tstellar
Copy link
Collaborator

Hi Hal,

What is your opinion on backporting this?

https://reviews.llvm.org/rG25bbff632d018d178272a61c0732203d53d3a2e3

@ahsansaghir
Copy link
Contributor Author

The fix does not apply cleanly, could someone backport this and push a
branch to their local github fork?

Hi Tom, sorry for the late response. I will work on it to get you a patch that applies cleanly.

Thanks!

@ahsansaghir
Copy link
Contributor Author

Hi Tom,
I am not aware of the procedure for backporting a patch. I would need some help with that. Is there a branch for 12.0.1 that I can apply my patch to and perhaps upload the diff for the applied patch?

Thanks!

@ahsansaghir
Copy link
Contributor Author

Hi Hal,

What is your opinion on backporting this?

https://reviews.llvm.org/rG25bbff632d018d178272a61c0732203d53d3a2e3

Nemanja suggested to backport this.

@ahsansaghir
Copy link
Contributor Author

Hi Tom,
I am not aware of the procedure for backporting a patch. I would need
some help with that. Is there a branch for 12.0.1 that I can apply my patch
to and perhaps upload the diff for the applied patch?

Thanks!

Hi Tom,
I created a branch off of release/12.x and cherry-picked the fix (25bbff6) and it applied cleanly for me. May be I am missing something. Can you please let me know if I am missing something? If not, can you please give it a try to cherry-pick the fix again. Thanks!

@tstellar
Copy link
Collaborator

I'm still trying to verify this with the abi-dumper tool, but I think adding these builtins changes the API/ABI, because the values of the builtin enum change. Is there some way to add these alias without adding the enums to BuiltinsPPC.def ?

@ahsansaghir
Copy link
Contributor Author

I'm still trying to verify this with the abi-dumper tool, but I think adding
these builtins changes the API/ABI, because the values of the builtin enum
change. Is there some way to add these alias without adding the enums to
BuiltinsPPC.def ?

I discussed this with Nemanja and he suggested that if this is causing a significant issue to backport, we should not backport it.
Thanks for your effort Tom, appreciated!

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
@xgupta
Copy link
Contributor

xgupta commented Jan 22, 2023

We can close this issue, as the patch is committed.

@xgupta xgupta closed this as completed Jan 22, 2023
@EugeneZelenko EugeneZelenko added clang:headers Headers provided by Clang, e.g. for intrinsics and removed c++ labels Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla clang:headers Headers provided by Clang, e.g. for intrinsics
Projects
None yet
Development

No branches or pull requests

4 participants