[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3136cbe29e74: [PowerPC] Implement Vector Shift Builtins (authored by Conanap). Changed prior to commit: https://reviews.llvm.org/D83338?vs=283980

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

amyk accepted this revision. amyk added a comment. This revision is now accepted and ready to land. Thanks for addressing the comments. LGTM. Comment at: llvm/test/CodeGen/PowerPC/p10-vector-shift.ll:10 + +; These tests ensure that vector shift quadword builtins are correctly +

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

Conanap added inline comments. Comment at: clang/lib/Headers/altivec.h:17243 + // return (vector signed __int128)vec_sr((vector unsigned __int128)__a, __b); + return (vector signed __int128)( +((vector unsigned __int128) __a) >> amyk wrote: > Could

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

Conanap updated this revision to Diff 283980. Conanap marked 8 inline comments as done. Conanap added a comment. Added shl tests, formatting fixes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83338/new/ https://reviews.llvm.org/D83338 Files: cl

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

amyk added inline comments. Comment at: llvm/lib/Target/PowerPC/PPCInstrPrefix.td:1144 +(v1i128 (VSLQ v1i128:$VRA, v1i128:$VRB))>; + def : Pat<(v1i128 (PPCshl v1i128:$VRA, v1i128:$VRB)), +(v1i128 (VSLQ v1i128:$VRA, v1i128:$VRB))>; amyk wr

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

amyk added inline comments. Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1100 + +if (Subtarget.isISA3_1()) { + setOperationAction(ISD::SRA, MVT::v1i128, Legal); amyk wrote: > No brackets are needed here. > > Also, I think it might make sense

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

amyk added inline comments. Comment at: clang/lib/Headers/altivec.h:17217 + +/* vs[l | r | ra] */ +static __inline__ vector unsigned __int128 __ATTRS_o_ai Add a space after this comment. Comment at: clang/lib/Headers/altivec.h:17227 +vec_sl(vec

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

Conanap updated this revision to Diff 281783. Conanap added a comment. Replaced a forgotten test file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83338/new/ https://reviews.llvm.org/D83338 Files: clang/lib/Headers/altivec.h clang/test/CodeG

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

Conanap updated this revision to Diff 281735. Conanap added a comment. Fixed formatting based on bot feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83338/new/ https://reviews.llvm.org/D83338 Files: clang/lib/Headers/altivec.h clang/te

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

lei added a comment. Please address the auto generated clang-format issues for the added code in this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83338/new/ https://reviews.llvm.org/D83338 __

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

Conanap updated this revision to Diff 280592. Conanap removed a reviewer: power-llvm-team. Conanap added a comment. Herald added a subscriber: kbarton. Converted the impelmentation to an open coded implementation and updated the test cases as appropriate. Repository: rG LLVM Github Monorepo

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

amyk added a comment. I realize it may be possible to open code these, as these functions already exist in altivec.h. Could you look into if this is the case? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83338/new/ https://reviews.llvm.org/D83338

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

Conanap updated this revision to Diff 279348. Conanap marked 8 inline comments as done. Conanap added a comment. Fixed function names, test case clean up Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83338/new/ https://reviews.llvm.org/D83338 File

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

amyk added a comment. Please also change the function names. Comment at: clang/lib/Headers/altivec.h:17151 + +/* vs[l | r | raq] */ +static __inline__ vector unsigned __int128 __ATTRS_o_ai Actually, sorry, I think this comment should be the following instead: `

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

NeHuang added a comment. Overall seems fine to me, but of course, please wait to hear from Amy. Just some nits for the test case. Comment at: llvm/test/CodeGen/PowerPC/p10-vector-shift.ll:9 +; RUN: FileCheck %s + +define dso_local <1 x i128> @test_vec_slq(<1 x i128> %a, <1 x

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

stefanp added a comment. Just a few nits for this patch. Comment at: clang/lib/Headers/altivec.h:17158 +static __inline__ vector signed __int128 __ATTRS_o_ai +vec_sl(vector signed __int128 __a, vector unsigned __int128 __b) { + return __builtin_altivec_vslq((vector unsigned __

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

Conanap updated this revision to Diff 278235. Conanap marked 7 inline comments as done. Conanap added a comment. Formatting fixes, fixed test case return type, updated builtins' signatures to correct signatures. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

amyk added inline comments. Comment at: clang/lib/Headers/altivec.h:17153 +/* vs[l | r | raq] */ +static __inline__ vector unsigned __int128 __ATTRS_o_ai +vec_slq(vector unsigned __int128 __a, vector unsigned __int128 __b) { I believe there are supposed to be `si

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

amyk added inline comments. Comment at: clang/lib/Headers/altivec.h:17151 + +/* vector shifts for quadwords */ +/* vs[l | r | raq] */ I think we can remove `/* vector shifts for quadwords */`. Then, we can add a new line after `/* vs[l | r | raq] */` for consist

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

Conanap added a comment. Also removed instr def as it will be part of D83516 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83338/new/ https://reviews.llvm.org/D83338 ___

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

Conanap updated this revision to Diff 277896. Conanap marked 3 inline comments as done. Conanap added a comment. Added tests, added extra comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83338/new/ https://reviews.llvm.org/D83338 Files: c

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

amyk requested changes to this revision. amyk added a comment. This revision now requires changes to proceed. This will need to be rebased against your 2608 instruction definitions patch. But yes, I believe you are missing the clang and llc test case for this patch. Requesting changes due to mis

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

bsaleil added a comment. Shouldn't we have test cases to test `vec_sl`, `vec_sr` and `vec_sra` ? Comment at: llvm/include/llvm/IR/IntrinsicsPowerPC.td:800 def int_ppc_altivec_vsrw : PowerPC_Vec_WWW_Intrinsic<"vsrw">; +def int_ppc_altivec_vsrq : PowerPC_Vec_QQQ_Intrinsic<"vs

[PATCH] D83338: [PowerPC][Power10] Implemented Vector Shift Builtins

Conanap created this revision. Conanap added reviewers: PowerPC, power-llvm-team, saghir, nemanjai, hfinkel. Conanap added projects: LLVM, clang, PowerPC. Implemented the following vector right and left shift builtins and its test cases: vector unsigned __int128 vec_sl(vector unsigned __int128