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

[Amy Kwan via Phabricator via llvm-commits]

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(vector signed __int128 __a, vector unsigned __int128 __b) {
+  // return (vector signed __int128)vec_sl((vector unsigned __int128)__a, __b);
+  return __a << (__b %
----------------
Please remove any commented code.


================
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) >>
----------------
Could you please fix the indentation of the returns to make them all consistent?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1100
+
+    if (Subtarget.isISA3_1()) {
+      setOperationAction(ISD::SRA, MVT::v1i128, Legal);
----------------
No brackets are needed here.

Also, I think it might make sense to move this block into the previous `hasP9Altivec` condition since in there it has:
```
      setOperationAction(ISD::SHL, MVT::v1i128, Legal);
      setOperationAction(ISD::SRL, MVT::v1i128, Legal);
```


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrPrefix.td:1141
+/* Vector shifts for ISA3_1 */
+let Predicates = [IsISA3_1] in {
+  def : Pat<(v1i128 (shl v1i128:$VRA, v1i128:$VRB)),
----------------
There is no need to make a new predicate block, you can put these anonymous patterns in the block above.


================
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))>;
----------------
I noticed that we have patterns for the PPCISD nodes, but I think no tests to show these patterns?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83338/new/

https://reviews.llvm.org/D83338