[PATCH] D83500: [PowerPC][Power10] Implement custom codegen for the vec_replace_elt and vec_replace_unaligned builtins.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14267
+    assert((ArgWidth == 32 || ArgWidth == 64) && "Invalid argument width");
+    if (BuiltinID == PPC::BI__builtin_altivec_vec_replace_elt)
+      ConstArg *= ArgWidth / 8;
----------------
`// The input to vec_replace_elt is an element index, not a byte index.`


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14277
+      Ops[2] = ConstantInt::getSigned(Int32Ty, ConstArg);
+      // Perform additional handling if the second argument is a float.
+      if (!Ops[1]->getType()->isIntegerTy(32)) {
----------------
This is too vague.
`// If the input vector is a float type, bitcast the inputs to integers.`


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14278
+      // Perform additional handling if the second argument is a float.
+      if (!Ops[1]->getType()->isIntegerTy(32)) {
+        Ops[0] = Builder.CreateBitCast(Ops[0],
----------------
This seems to be duplicated in both blocks. Can we not just do something like
`if (!Ops[1]->getType()->isIntegerTy(ArgWidth))`? Then inside we can use the ternary operator to select between `Int32Ty` and `Int64Ty` if necessary.
Then we only need one of these bitcast blocks just before we emit the call.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14291
+      Ops[2] = ConstantInt::getSigned(Int32Ty, ConstArg);
+      // Perform additional handling if the second argument is a double.
+      if (!Ops[1]->getType()->isIntegerTy(64)) {
----------------
More specific comment please - just as above.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14300
+    Call = Builder.CreateCall(F, Ops);
+    // Depending on the builtin, bitcast to the approriate resultant type.
+    if (BuiltinID == PPC::BI__builtin_altivec_vec_replace_elt &&
----------------
s/resultant/result


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3196
+  // the same type as the element type of the first vector argument.
+  auto SemaVecInsertCheck = [&](CallExpr *TheCall) -> bool {
+    Expr *VectorArg = TheCall->getArg(0);
----------------
I am very surprised that this doesn't exist already but it seems more useful to have a `static` function in this file along the lines of:
`static bool isEltOfVectorTy(QualType VectorTy, QualType EltTy)`

That would do the obvious check.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:3245
+    QualType VecInsArgTy = TheCall->getArg(1)->getType();
+    if (Context.getIntWidth(VecInsArgTy) == 32) // Check the range for vinsw.
+      return SemaBuiltinConstantArgRange(TheCall, 2, 0, 12) ||
----------------
I don't think the `if` statements add to readability. I think this should just be a single return statement and the range should be selected by a ternary op.
Something like:
```
unsigned Width = Context.getIntWidth(TheCall->getArg(1)->getType());
QualType VecTy = TheCall->getArg(0)->getType();
QualType EltTy = TheCall->getArg(1)->getType();
return SemaBuiltinConstantArgRange(TheCall, 2, 0, Width == 32 ? 12 : 8) ||
  isEltOfVectorTy(VecTy, EltTy);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83500