This is an archive of the discontinued LLVM Phabricator instance.

[clang][AArch64][SVE] Implicit conversions for vector-scalar operations
AbandonedPublic

Authored by DavidTruby on May 3 2022, 9:32 AM.

Details

Summary

This patch allows the same implicit conversions for vector-scalar
operations in SVE that are allowed for NEON.

Diff Detail

Event Timeline

DavidTruby created this revision.May 3 2022, 9:32 AM
DavidTruby requested review of this revision.May 3 2022, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2022, 9:32 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Matt added a subscriber: Matt.May 3 2022, 2:08 PM

Looking pretty good. A couple of test cases to consider:

#include <arm_neon.h>
#include <arm_sve.h>
svint8_t svi8(svint8_t a) {
          return a + 256;
}
int8x16_t nei8(int8x16_t a) {
        return a + 256;
}
svint8_t svi8_128(svint8_t a) {
          return a + 128;
}
int8x16_t nei8_128(int8x16_t a) {
        return a + 128;
}
inp.c:4:13: error: invalid operands to binary expression ('svint8_t' (aka '__SVInt8_t') and 'int')
          return a + 256;
                 ~ ^ ~~~
inp.c:7:11: error: cannot convert between scalar type 'int' and vector type 'int8x16_t' (vector of 16 'int8_t' values) as implicit conversion would cause truncation
        return a + 256;
                 ^
inp.c:13:13: warning: implicit conversion from 'int' to 'int8x16_t' (vector of 16 'int8_t' values) changes value from 128 to -128 [-Wconstant-conversion]
        return a + 128;
                 ~ ^~~

Note that for NEON we get a warning about the implicit conversion, and a clear error message about truncation, but for SVE there is no warning and simply an 'invalid operands to binary expression', which could be confusing since the implicit conversions exist for the same operand type with differing value.

Improved diagnostics

clang/lib/Sema/SemaExpr.cpp
10588

!Ptr && [Deref Ptr] looks suspicious. Also, if I insert an abort in this path and run check-clang I don't see any failing tests?

It might help to name LHSBuiltinTy && LHSBuiltinTy->isVLSTBuiltinType() IsLHSVector (likewise for RHS) or similar.

Fix assertion failures

I think this needs more tests to ensure every diagnostic is being triggered correctly so I will be adding those in another patch, but I wanted to put up the fix for the original issue as soon as possible!

Add additional tests and fix bugs associated with new tests

can the diagnostics improvements be broke out into separate patches? I think it would make this easier to review

can the diagnostics improvements be broke out into separate patches? I think it would make this easier to review

I've split the diagnostic changes from this out as D126377 and will repost this patch soon based on that one. Thanks!

DavidTruby abandoned this revision.May 25 2022, 7:33 AM

I've resubmitted this split from the diagnostic changes as D126380