search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

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

Open Quuxplusone opened 3 years ago

Quuxplusone commented 3 years ago
Bugzilla Link PR50159
Status REOPENED
Importance P release blocker
Reported by Ahsan Saghir (saghir.ibm@gmail.com)
Reported on 2021-04-28 18:51:43 -0700
Last modified on 2021-05-20 12:07:59 -0700
Version trunk
Hardware PC Linux
CC blitzrakete@gmail.com, dgregor@apple.com, erik.pilkington@gmail.com, llvm-bugs@lists.llvm.org, lukebenes@hotmail.com, nemanja.i.ibm@gmail.com, richard-llvm@metafoo.co.uk, tstellar@redhat.com
Fixed by commit(s) rG25bbff632d018d178272a61c0732203d53d3a2e3
Attachments
Blocks PR49317
Blocked by
See also 
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
Quuxplusone commented 3 years ago 
(In reply to Ahsan Saghir from comment #0)
> 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?
Quuxplusone commented 3 years ago 
Sorry, missed the link for the patch:
Differential Revision: https://reviews.llvm.org/D100482
Quuxplusone commented 3 years ago

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.

Quuxplusone commented 3 years ago

Fix checked in: https://reviews.llvm.org/rG25bbff632d018d178272a61c0732203d53d3a2e3

Quuxplusone commented 3 years ago

Reopening for 12.x backport

Quuxplusone commented 3 years ago

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

Quuxplusone commented 3 years ago

Hi Hal,

What is your opinion on backporting this?

https://reviews.llvm.org/rG25bbff632d018d178272a61c0732203d53d3a2e3

Quuxplusone commented 3 years ago 
(In reply to Tom Stellard from comment #6)
> 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!
Quuxplusone commented 3 years ago 
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!
Quuxplusone commented 3 years ago 
(In reply to Tom Stellard from comment #7)
> Hi Hal,
>
> What is your opinion on backporting this?
>
> https://reviews.llvm.org/rG25bbff632d018d178272a61c0732203d53d3a2e3

Nemanja suggested to backport this.
Quuxplusone commented 3 years ago 
(In reply to Ahsan Saghir from comment #9)
> 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
(25bbff632d018d178272a61c0732203d53d3a2e3) 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!
Quuxplusone commented 3 years ago

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 ?

Quuxplusone commented 3 years ago 
(In reply to Tom Stellard from comment #12)
> 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!