[clang,ARM] Default to -fno-lax-vector-conversions in ARM v8.1-M.

[simon_tatham]

Summary

The ACLE intrinsics for the MVE instruction set have polymorphic
variants: you can write vaddq(v,w) and have it automatically select
vaddq_s32, vaddq_u8, vaddq_f16, ... according to the types of the
vector variables v and w.

In order to implement this using attribute((overloadable)), I need
to turn on strict type-checking between the different vector types, or
else clang will believe that any vector type matches _all_ of those
function prototypes, and won't be able to select the right one.

Also, I think this makes sense for MVE anyway, on error-checking
grounds. For NEON, where every intrinsic in C source explicitly
describes the operation it's performing, you could argue that the
distinct vector types aren't essential; but in a context where getting
the wrong type can cause silent generation of the _wrong_ code, I
think it's better to warn users as soon as possible if they've written
code that isn't using vector types consistently.

So this commit introduces a (minimal) piece of infrastructure to allow
the default for OPT_flax_vector_conversions to vary based on the
target triple, and uses it to set the default specially for
ARMSubArch_v8_1m_mainline (in which the only possible vector
architecture will be MVE).

[ostannard]

Test?

[simon_tatham]

Added a test.

[ostannard]

LGTM

[dmgreen]

FYI: rL371817, in case it changes what is done here.

[simon_tatham]

Rebased to current master.

[kristof.beyls]

Would it still make sense to have this patch after D68683 lands? At first sight, it seems this patch might no longer make sense then?

[dmgreen]

Yes.. My understanding is that the default is still -flax-vector-convertions=all (the old clang behaviour), but the plan is to change it (for all architectures) to none.

[simon_tatham]

Yes, the commit message in rL371817 mentions that the long-term aim is to change the default to none for everyone. If that change happens before I manage to land this patch, then this patch certainly will become unnecessary.

But D68683 doesn't make that change: it only prepares arm_neon.h not to fall over in future when the change does happen. And MVE needs strict vector type checking more urgently than anyone else (because for us, it's not just a nice-to-have error-checking improvement, but critical to the polymorphic ACLE intrinsics working properly in the first place).

[dmgreen]

Yeah, OK. D67678 is the patch that will change the default, but only to "int", not to "none" for the moment.

An earlier version of that patch had a different way of setting the default, in lib/Basic/Targets/ARM.cpp. I'm not sure that's better or worse than this, they both look fine to me.

Under that condition, LGTM!

[rsmith]

It is not reasonable to make this change on a per-target basis. We should work towards turning lax vector conversions off globally instead.

If overload resolution can't distinguish between overloads that perform a lax vector conversions and those that do not, we should fix that in overload resolution rather than papering over it with a change of per-target default -- this target's headers should still work even when users turn lax vector conversions back on.

[simon_tatham]

Hmmm. I take your point, and so I backed out my local version of this patch in order to try to solve the problem a different way in overload resolution.

But in fact overloading on different vector types already seems to work fine: my in-progress patch series still passes all the clang tests even without this change to the defaults. So unless there's been a fix to overload resolution since I started developing all of this (I looked, and didn't find one), I can't quite work out why I needed this patch in the first place now. It may be that it's completely unnecessary, and that I managed to fool myself early in development by misinterpreting some other kind of mistake I'd made.

[simon_tatham]

In D67160#1704840, @rsmith wrote:

If overload resolution can't distinguish between overloads that perform a lax vector conversions and those that do not, we should fix that in overload resolution rather than papering over it with a change of per-target default

I'm a bit late coming back to this, but now I've remembered why I wanted this change, and why clang's normal overload resolution doesn't do quite what the MVE polymorphic intrinsics want.

I'm in the middle of writing implementations of the vsetq_lane family of intrinsics. These take a vector as input, a scalar of the same type as the vector lane, and a lane index, and returns a modified vector with the specified lane replaced by the scalar input value. In other words, the overloaded versions consist of things like

int8x16_t vsetq_lane(int8_t, int8x16_t, int);
int16x8_t vsetq_lane(int16_t, int16x8_t, int);
int32x4_t vsetq_lane(int32_t, int32x4_t, int);

... and so on for unsigned and floating types too.
Now if the user calls

myvector = vsetq_lane(23, myvector, 1);

then I think they reasonably expect the choice of polymorphic intrinsic to be based on the type of the vector parameter, because the integer literal 23 in the scalar parameter could have been intended to be any of those integer types. But the strict rules of C say that it has type int (which in this case matches int32_t). So if you do this with myvector being (say) an int8x16_t, you get an error message complaining that the call is ambiguous: the vector parameter matches one of those prototypes, but the scalar parameter matches another, and neither one matches both.

Ideally I'd like to be able to configure this particular family of overloaded functions to give strictly higher priority to the vector type than the scalar type, in resolving this ambiguity. Turning off lax vector conversions has that effect, but I do agree with you that it would be better not to do it that way.

So, do you have any thoughts on a better approach? The only one I've so far thought of is to add extra spurious overloaded declarations in the header file for integer types that things might have accidentally been promoted to.

[simon_tatham]

(The draft implementations of vsetq_lane are now in D70133.)

[simon_tatham]

I ended up solving this problem a completely different way, in D72518. Instead of controlling the behavior I need based on the target architecture, I made it depend on a type attribute on the vector types, so that the MVE header file adds that attribute but nobody else does. And doing it like that I was also able to make the behavior more subtle, so that it makes the polymorphic MVE intrinsics work without also turning every case of lax vector checking into strict – so users still get to make a choice about how strict they want their vectors to be, and the intrinsics work either way.

clang/lib/Driver/ToolChains/Clang.cpp

static bool isSignedCharDefault(const llvm::Triple &Triple) {
   case llvm::Triple::riscv32:
   case llvm::Triple::riscv64:
   case llvm::Triple::systemz:
   case llvm::Triple::xcore:
     return false;
   }
 }
 
+static bool isLaxVectorConversionsDefault(const llvm::Triple &Triple) {
+  switch (Triple.getArch()) {
+  default:
+    return true;
+
+  case llvm::Triple::thumb:
+  case llvm::Triple::thumbeb:
+    return Triple.getSubArch() != llvm::Triple::ARMSubArch_v8_1m_mainline;
+  }
+}
+
 static bool isNoCommonDefault(const llvm::Triple &Triple) {
   switch (Triple.getArch()) {
   default:
     if (Triple.isOSFuchsia())
       return true;
     return false;
 
   case llvm::Triple::xcore:
#endif
   XRay.addArgs(TC, Args, CmdArgs, InputType);
 
   if (TC.SupportsProfiling())
     Args.AddLastArg(CmdArgs, options::OPT_pg);
 
   if (TC.SupportsProfiling())
     Args.AddLastArg(CmdArgs, options::OPT_mfentry);
 
-  // -flax-vector-conversions is default.
-  if (!Args.hasFlag(options::OPT_flax_vector_conversions,
-                    options::OPT_fno_lax_vector_conversions))
+  if (const Arg *A = Args.getLastArg(options::OPT_flax_vector_conversions,
+                    options::OPT_fno_lax_vector_conversions)) {
+    if (A->getOption().matches(options::OPT_fno_lax_vector_conversions))
       CmdArgs.push_back("-fno-lax-vector-conversions");
+  } else if (!isLaxVectorConversionsDefault(Triple)) {
+    CmdArgs.push_back("-fno-lax-vector-conversions");
+  }
 
   if (Args.getLastArg(options::OPT_fapple_kext) ||
       (Args.hasArg(options::OPT_mkernel) && types::isCXX(InputType)))
     CmdArgs.push_back("-fapple-kext");
 
   Args.AddLastArg(CmdArgs, options::OPT_fobjc_sender_dependent_dispatch);
   Args.AddLastArg(CmdArgs, options::OPT_fdiagnostics_print_source_range_info);
   Args.AddLastArg(CmdArgs, options::OPT_fdiagnostics_parseable_fixits);

clang/test/Driver/lax-vector-conversions.c

+// RUN: %clang --target=arm-arm-none-eabi -march=armv8.1m.main -### -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-STRICT %s
+
+// RUN: %clang --target=arm-arm-none-eabi -march=armv8a -### -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-LAX %s
+
+// CHECK-STRICT: "-fno-lax-vector-conversions"
+// CHECK-LAX-NOT: "-fno-lax-vector-conversions"