XL considers different vector types to be incompatible with each other.
For example assignment between variables of types vector float and vector
long long or even vector signed int and vector unsigned int are diagnosed.
clang, however does not diagnose such cases and does a simple bitcast between
the two types. This could easily result in program errors. This patch is to
fix the implicit casts in altivec.h so that there is no incompatible vector
type errors whit -fno-lax-vector-conversions, this is the prerequisite patch
to switch the default to -fno-lax-vector-conversions later.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Also, please run clang-format on the changes.
clang/lib/Headers/altivec.h | ||
---|---|---|
19203 | We should never cast anything to an integral vector type that doesn't include signed/unsigned/bool. | |
clang/test/CodeGen/PowerPC/builtins-ppc-p10vector.c | ||
2155 | This is not good. We are changing semantics here - turning a logical shift into an arithmetic shift. | |
clang/test/CodeGen/PowerPC/builtins-ppc-quadword-noi128.c | ||
10 | Why is this addition needed? Seems like something is wrong if we need to force enable int128 in order to compile something with altivec.h. |
I was wondering where are the test cases in this patch. Did they get missed when updating the revision?
The test case's modification were not needed as part of addressing Nemanja's comments.
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
15587 | This is strange. You don't need the ternary operator when you are simply setting a bool variable. This is simply: bool isUnaligned = (BuiltinID == PPC::BI__builtin_altivec_vinsw || BuiltinID == PPC::BI__builtin_altivec_vinsd); Same below. Also, please be careful of the naming convention (i.e. variables start with upper case, functions with lowercase, etc.). I will not add further comments to this end but please apply this change to the entire patch. | |
15600 | This is certainly concise, but trying to parse it puts my brain into an infinite loop. Please write it as: int ValidMaxValue = 0; if (IsUnaligned) ValidMaxValue = ... ? ... : ... else ValidMaxValue = ... | |
15604 | Since you are building a string from pieces here, might as well mention the value the user specified so it is something like: | |
15664 | Why not simplify the bitcasts below by just creating a pointer to the vector type (call it say, V1I128Ty) here? Same in the next case. | |
clang/lib/Headers/altivec.h | ||
3204–3219 | This is wrong. Please see the documentation for what the result type should be under XL compat. I believe it needs to be vector float regardless of the input. | |
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat.c | ||
8 | I don't know why only one of the functions below has checks for this run line, but that needs to be fixed. Please add the NOCOMPAT checks on the other functions. | |
30 | We compute a vector float but return a vector double. Something is wrong. Please see my comment regarding this in altivec.h. | |
llvm/include/llvm/IR/IntrinsicsPowerPC.td | ||
746 ↗ | (On Diff #437542) | What happened here? Unrelated whitespace change. Please remove it before committing. |
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat.c | ||
---|---|---|
8 | Can we address this on a followup patch? Since it's not related to the work here. |
This is strange. You don't need the ternary operator when you are simply setting a bool variable. This is simply:
Same below.
Also, please be careful of the naming convention (i.e. variables start with upper case, functions with lowercase, etc.). I will not add further comments to this end but please apply this change to the entire patch.