[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 requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

The description includes `... however it is more preferable to use bitcast`. It is not a question of preference but of correctness. The fp to int conversions truncate while bitcasts don't. The semantics of the builtins require that no truncation happen.

Also, please include checks in SemaChecking for:

- Third argument being constant
- Third argument being within range
- Second argument having the same type as the element type of the first



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14275
+    ConstantInt *ArgCI = dyn_cast<ConstantInt>(Ops[2]);
+    assert(ArgCI &&
+           "Third Arg to vinsw/vinsd intrinsic must be a constant integer!");
----------------
Where is the code that ensures this? There does not appear to be a Sema check to emit a meaningful message for this. We also need a test with a non-constant argument to show the message.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14278
+    llvm::Type *ResultType = ConvertType(E->getType());
+    llvm::Function *F = CGM.getIntrinsic(Intrinsic::ppc_altivec_vinsw);
+    int64_t ConstArg = ArgCI->getSExtValue();
----------------
I don't think we should be creating the declaration if we may not use it. Just initialize this to `nullptr` here and set it for each case.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14307
+      // Perform additional handling if the second argument is a double.
+      if (Ops[1]->getType()->isDoubleTy()) {
+        Ops[0] = Builder.CreateBitCast(Ops[0],
----------------
Please change this to a negative condition (i.e. if the type is **not** `i64`). Similarly in other similar conditions.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14319
+  }
+  case PPC::BI__builtin_altivec_vec_replace_unaligned: {
+    // The third argument of vec_replace_unaligned must be a compile time
----------------
Can we reorganize this as something like:
```
case PPC::BI__builtin_altivec_vec_replace_elt:
case PPC::BI__builtin_altivec_vec_replace_unaligned: {
  // Define variables that are needed
  unsigned ArgWidth = Ops[1]->getType()->getPrimitiveSizeInBits();
  if (BuiltinID == PPC::BI__builtin_altivec_vec_replace_elt)
    ConstArg *= ArgWidth / 8;
  assert((ArgWidth == 32 || ArgWidth == 64) && "Invalid argument width");
  if (ArgWidth == 32) {
    // set up what is needed for vinsw
  } else {
    // set up what is needed for vinsd
  }
  // Emit the call
  if (BuiltinID == PPC::BI__builtin_altivec_vec_replace_elt)
    // add the bitcast of the result
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83500