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