Page MenuHomePhabricator

[PowerPC] Fixing implicit castings in altivec for -fno-lax-vector-conversions
ClosedPublic

Authored by lei on Apr 20 2022, 7:54 AM.

Details

Summary

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.

Diff Detail

Event Timeline

maryammo created this revision.Apr 20 2022, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2022, 7:54 AM
maryammo requested review of this revision.Apr 20 2022, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2022, 7:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
maryammo updated this revision to Diff 424050.Apr 20 2022, 3:40 PM

[NFC] Fix the formatting

nemanjai requested changes to this revision.Apr 20 2022, 6:43 PM

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.

This revision now requires changes to proceed.Apr 20 2022, 6:43 PM
maryammo updated this revision to Diff 425086.Apr 25 2022, 6:03 PM

[PowerPC] Address the review comments

maryammo marked an inline comment as done.Apr 26 2022, 7:32 AM
amyk added a subscriber: amyk.May 20 2022, 3:15 PM

I was wondering where are the test cases in this patch. Did they get missed when updating the revision?

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.

maryammo updated this revision to Diff 437399.Wed, Jun 15, 4:45 PM

Updating test cases

lei updated this revision to Diff 437421.Wed, Jun 15, 7:05 PM

Add fix for vec_replace_elt and vec_replace_unaligned.

Herald added a project: Restricted Project. · View Herald TranscriptWed, Jun 15, 7:05 PM
lei updated this revision to Diff 437428.Wed, Jun 15, 8:06 PM

rebase to ToT

lei updated this revision to Diff 437515.Thu, Jun 16, 5:45 AM

Move error checking for vec_replace_unaligned to during code gen.

lei updated this revision to Diff 437542.Thu, Jun 16, 7:48 AM

Simplify vec_replace_[elt|unaligned] code gen.

nemanjai added inline comments.Thu, Jun 16, 9:35 AM
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:
byte number 44 is outside of the valid range [0, 12]

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.

lei commandeered this revision.Thu, Jun 16, 10:10 AM
lei edited reviewers, added: maryammo; removed: lei.
lei updated this revision to Diff 437627.Thu, Jun 16, 11:24 AM
lei marked 9 inline comments as done.

Address review comments from Nemanja

lei marked an inline comment as not done.Thu, Jun 16, 12:43 PM
lei added inline comments.
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.

nemanjai accepted this revision.Thu, Jun 16, 2:29 PM

LGTM. Thanks.

clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat.c
8

Absolutely.

This revision is now accepted and ready to land.Thu, Jun 16, 2:29 PM
amyk accepted this revision.Thu, Jun 16, 3:00 PM

This patch also LGTM. Thank you Lei.

This revision was landed with ongoing or failed builds.Thu, Jun 16, 3:07 PM
This revision was automatically updated to reflect the committed changes.