Page MenuHomePhabricator

[AArch64][SVE] Support implicit lax vector conversions for SVE types
ClosedPublic

Authored by joechrisellis on Nov 9 2020, 4:00 AM.

Details

Summary

Lax vector conversions was behaving incorrectly for implicit casts
between scalable and fixed-length vector types. For example, this:

#include <arm_sve.h>

#define N __ARM_FEATURE_SVE_BITS
#define FIXED_ATTR __attribute__((arm_sve_vector_bits(N)))

typedef svfloat32_t fixed_float32_t FIXED_ATTR;

void allowed_depending() {
  fixed_float32_t fs32;
  svfloat64_t s64;

  fs32 = s64;
}

... would fail because the vectors have differing lane sizes. This patch
implements the correct behaviour for
-flax-vector-conversions={none,all,integer}. Specifically:

  • -flax-vector-conversions=none prevents all lax vector conversions between scalable and fixed-sized vectors.
  • -flax-vector-conversions=integer allows lax vector conversions between scalable and fixed-size vectors whose element types are integers.
  • -flax-vector-conversions=all allows all lax vector conversions between scalable and fixed-size vectors (including those with floating point element types).

The implicit conversions are implemented as bitcasts.

Diff Detail

Event Timeline

joechrisellis created this revision.Nov 9 2020, 4:00 AM
joechrisellis requested review of this revision.Nov 9 2020, 4:00 AM

fixed_float64_t appears in the commit message but also is unused.

clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp
15 ↗(On Diff #303816)

I can't see any uses of fixed_float64_t and fixed_int64_t?

joechrisellis marked an inline comment as done.

Address @peterwaller-arm's comment regarding unused types.

clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp
15 ↗(On Diff #303816)

Good spot! Removed.

joechrisellis edited the summary of this revision. (Show Details)Nov 9 2020, 5:36 AM

Hi @joechrisellis - thank you for this patch!

I have left a couple of comments.

Francesco

clang/lib/AST/ASTContext.cpp
8584–8587

May I ask to avoid this triple if statement? Given that BT is not used after being defined, I think the following form would be easier to understand:

if (!FirstType->getAs<BuiltinType>())
  return false;

const auto *VT = SecondType->getAs<VectorType>();

if (VT && VT->getVectorKind() == VectorType::SveFixedLengthDataVector) {
   /// ...
    return ...
}

return false;

May I ask you to give meaningful names to the variables? BT and VT are quite cryptic to me.

Moreover.. are BT and VT really needed? You are asserting FirstType->isSizelessBuiltinType() && SecondType->isVectorType() ... the getAs calls should not fail, no? given that the lambda is local to this method, I wouldn't bother making it work for the generic case.

clang/lib/Sema/SemaCast.cpp
2222 ↗(On Diff #303838)

This code path seems untested.

clang/lib/Sema/SemaOverload.cpp
1650–1652

tabs!

clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp
19 ↗(On Diff #303838)

Why this one in particular? To me the comment would make more sense if saying

// An explicit cast is always allowed, irrespective of the value of -flax-vector-conversions.
joechrisellis marked 3 inline comments as done.

Address @fpetrogalli's comments.

joechrisellis marked an inline comment as done.Nov 11 2020, 4:06 AM
joechrisellis added inline comments.
clang/lib/AST/ASTContext.cpp
8584–8587

Simplified the code as per your suggestion, but the lambda here here serves a purpose: it makes sure that areLaxCompatibleSveTypes has the same behaviour irrespective of the ordering of the parameters. So we do actually need the if statements inside the lambda.

clang/lib/Sema/SemaCast.cpp
2222 ↗(On Diff #303838)

Thinking about it, this could do with being broken out into its own patch. Will do this.

clang/lib/Sema/SemaOverload.cpp
1650–1652

Not sure where these came from, since I ran clang-format over the patch. Think they should be gone now...

clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp
19 ↗(On Diff #303838)

Will break this out into another patch as mentioned above.

fpetrogalli accepted this revision.Nov 11 2020, 5:09 AM

Thank you @joechrisellis - LGTM!

This revision is now accepted and ready to land.Nov 11 2020, 5:09 AM
joechrisellis marked an inline comment as done.
  • Support C lax vector conversions.
  • Test C lax vector conversions.

Remove failing test; it was checking that a conversion _failed_, although the conversion should now _pass_ given the changes in this patch.

This revision was landed with ongoing or failed builds.Nov 17 2020, 6:50 AM
This revision was automatically updated to reflect the committed changes.