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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
