[TypeSize] Allow returning scalable size in implicit conversion to uint64_t

[sdesmalen]

Summary

This patch removes compiler runtime assertions that ensure the implicit
conversion are only guaranteed to work for fixed-width vectors.

With the assert it would be impossible to get _anything_ to build until the
entire codebase has been upgraded, even when the indiscriminate uses of
the size as uint64_t would work fine for both scalable and fixed-width
types.

This issue will need to be addressed differently, with build-time errors
rather than assertion failures, but that effort falls beyond the scope
of this patch.

Returning the scalable size and avoiding the assert in getFixedSize()
is a temporary stop-gap in order to use LLVM for compiling and using
the SVE ACLE intrinsics.

[ctetreau]

how pervasive is this issue? I worry that adding this stopgap today will allow a bunch of dangerous code to get committed, and will make future efforts to correct the issue more difficult.

Can the places where this currently blows up just call getKnownMinSize()?

[sdesmalen]

In D75297#1896735, @ctetreau wrote:

how pervasive is this issue? I worry that adding this stopgap today will allow a bunch of dangerous code to get committed, and will make future efforts to correct the issue more difficult.

We should probably move to a different approach soon (as we discussed in our last sync-up call), but until then I think there is little down-side to removing this assert. Removing the assert would allow us to use the SVE ACLE intrinsics which don't rely on common optimisations.
Until we get auto-vec support there are currently no other uses of scalable types. Moving to auto-vec will require a systematic approach to upgrading the code-base to distinguish scalable types, so we may have to fix up a couple more cases for new code that's added until we do so, but even with the assert nothing would break unless someone adds new code and/or tests that uses scalable types.

Can the places where this currently blows up just call getKnownMinSize()?

I'd rather not change all places to use getKnownMinSize() as this suggests the code is successfully ported.

[efriedma]

Removing the assert would allow us to use the SVE ACLE intrinsics which don't rely on common optimisations.

Even without any optimizations that are specifically SVE-aware, we're going to generate a lot of instructions with SVE types that aren't SVE intrinsics. In particular, I assume plain C assignment operations are going to lower to "load" and "store" instructions. So really, this is trading assertions for subtle miscompiles in a lot of cases.

If you're trying to hit a certain deadline, maybe being unable to build any SVE code at all is worse than a bunch of subtle miscompiles? That seems like a terrible tradeoff either way, though.

[ctetreau]

In D75297#1901266, @sdesmalen wrote:

We should probably move to a different approach soon (as we discussed in our last sync-up call), but until then I think there is little down-side to removing this assert. Removing the assert would allow us to use the SVE ACLE intrinsics which don't rely on common optimisations.
Until we get auto-vec support there are currently no other uses of scalable types. Moving to auto-vec will require a systematic approach to upgrading the code-base to distinguish scalable types, so we may have to fix up a couple more cases for new code that's added until we do so, but even with the assert nothing would break unless someone adds new code and/or tests that uses scalable types.

I have concerns about introducing technical debt that we plan to pay off "soon". Life happens and things get deemed "good enough". If this patch is necessary for work to proceed on ACLE, perhaps those working on it should locally cherry pick this patch and do their work?

Currently, this implicit conversion is an alias for getFixedSize(). Lots of code is written using this assumption, and changing the meaning of the conversion is only going to add bugs. In my opinion, the conversion should be removed; if this had been done previously then we wouldn't be in this position today. Failing that, we should fix the breakages, not hide them.

[sdesmalen]

In D75297#1901560, @efriedma wrote:

Even without any optimizations that are specifically SVE-aware, we're going to generate a lot of instructions with SVE types that aren't SVE intrinsics. In particular, I assume plain C assignment operations are going to lower to "load" and "store" instructions. So really, this is trading assertions for subtle miscompiles in a lot of cases.

That's right. Plain assignment operations will use the unpredicated load/store instructions. There are definitely optimiziations for those that we'd need to disable for scalable vectors.

If you're trying to hit a certain deadline, maybe being unable to build any SVE code at all is worse than a bunch of subtle miscompiles? That seems like a terrible tradeoff either way, though.

It's also about making progress on getting any support for SVE/SVE2 in Clang/LLVM whilst in the meantime addressing the fixed-width assumptions more structurally. The one doesn't necessarily have to block the other, we can do this in parallel. At the moment scalable vectors are not yet actually supported in LLVM, so it is not unexpected that things are broken while we work towards adding support for it.
I agree though that the trade-off is not the most comfortable one, but by minimising the use of common parts of LLVM by implementing most functionality with custom intrinsics, a lot of that risk can be mitigated for the ACLE.

In D75297#1901615, @ctetreau wrote:

I have concerns about introducing technical debt that we plan to pay off "soon". Life happens and things get deemed "good enough". If this patch is necessary for work to proceed on ACLE, perhaps those working on it should locally cherry pick this patch and do their work?

It depends on what you mean with proceeding. Keeping the assert means that LLVM can't be used for scalable vectors until we've fully upgraded the code-base to use the proper interfaces to TypeSize (and ElementCount, given D75478), which is a really significant migration effort. The upgrading also becomes harder than it needs to be because tests need to be written that don't trigger the assert. In the worst case, we may not be able to write any tests until significant upgrading has occurred (I think we've already seen this in some cases).

Currently, this implicit conversion is an alias for getFixedSize(). Lots of code is written using this assumption, and changing the meaning of the conversion is only going to add bugs. In my opinion, the conversion should be removed; if this had been done previously then we wouldn't be in this position today. Failing that, we should fix the breakages, not hide them.

Yes, I agree we should remove the conversion, the question is more how we go about doing that. It is a trade-off between working until we've fixed up all uses of getSize() before we can build a simple example, vs gradually making an implementation we have more stable.

[efriedma]

We've found that having the assertion makes it much easier to track down specific callers. That means we don't get some weird crash/verifier failure/miscompile later on, we can be reasonably sure that we aren't hitting an issue with this if the assertion doesn't trigger, and we accumulate testcases for various codepaths. So I'd like to try to keep the assertion if possible.

If it's blocking your work on the intrinsics, though, that's worse.

[ctetreau]

I've said my piece; I'm fairly strongly opposed to this change because I feel that it will turn obvious bugs into subtle bugs. That said, if you can convince Eli that this is fine then I'll defer to his judgement.

[sdesmalen]

• Added cmake option to enable use of getFixedSize() in implicit conversion of TypeSize to uint64_t, which triggers an assertion failure when the size is scalable.
• Added deprecation warning for the implicit conversion of TypeSize->uint64_t.

[sdesmalen]

I updated the patch to reflect the discussion in the sync-up call earlier. The LLVM_ATTRIBUTE_DEPRECATED causes a lot of deprecated warnings though. These warnings are of course easy to silence, but I'm not sure how palatable that is for others?

[efriedma]

llvm/include/llvm/Support/TypeSize.h

165

Do deprecated warnings trigger on the LLVM codebase itself? I think producing a ton of warnings is going to cause issues.

[efriedma]

Yes, producing warnings by default is going to be a problem. One, it's going to really annoy everyone else, and two, some bots build with -Werror.

[sdesmalen]

• Replaced deprecated warning (at LLVM compile time) with runtime warning message.

[efriedma]

LGTM

llvm/CMakeLists.txt

426

80 columns

llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp

608 ↗

(On Diff #248763)

Unrelated change?

[sdesmalen]

• Removed isDigit from AMDGPULibFunc.cpp to use the one defined in StringExtras.h instead.

llvm/CMakeLists.txt

426

The convention in this file doesn't seem to honour 80 chars. I've put the message on a separate line similar to how this is done for other options.

llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp

608 ↗

(On Diff #248763)

llvm/Support/WithColor.h indirectly includes llvm/ADT/StringExtras.h which also defines a isDigit function. That makes this call ambiguous with the isDigit defined in this file.
The better fix is probably to remove the local isDigit and include StringExtras.h in this file directly.

[ctetreau]

llvm/CMakeLists.txt

426

No need to change it unless you need to upload a patch for some other reason, but I believe the policy is to run clang-format on lines that you touch. The fact that this policy creates local inconsistency is a separate flame war we need not get into here.

I often get a bot that complains when I mess this up elsewhere in the codebase, but it doesn't seem to run consistently for some reason...

[sdesmalen]

llvm/CMakeLists.txt

426

Okay thanks, I wasn't aware that was the official policy. For this specific case though, clang-format doesn't seem to apply to CMake files.

llvm/CMakeLists.txt

 if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
   option(LLVM_ENABLE_ASSERTIONS "Enable assertions" OFF)
 else()
   option(LLVM_ENABLE_ASSERTIONS "Enable assertions" ON)
 endif()
 
 option(LLVM_ENABLE_EXPENSIVE_CHECKS "Enable expensive checks" OFF)
 
+# While adding scalable vector support to LLVM, we temporarily want to
+# allow an implicit conversion of TypeSize to uint64_t. This CMake flag
+# enables a more strict conversion where it asserts that the type is not
+# a scalable vector type.
+#
+# Enabling this flag makes it easier to find cases where the compiler makes
+# assumptions on the size being 'fixed size', when building tests for
+# SVE/SVE2 or other scalable vector architectures.
+option(LLVM_ENABLE_STRICT_IMPLICIT_CONVERSION_TYPESIZE "Enable assertions that type is not scalable in implicit conversion from TypeSize to uint64_t" OFF)
+
 set(LLVM_ABI_BREAKING_CHECKS "WITH_ASSERTS" CACHE STRING
   "Enable abi-breaking checks.  Can be WITH_ASSERTS, FORCE_ON or FORCE_OFF.")
 
 option(LLVM_FORCE_USE_OLD_TOOLCHAIN
        "Set to ON to force using an old, unsupported host toolchain." OFF)
 
 option(LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN
        "Set to ON to only warn when using a toolchain which is about to be deprecated, instead of emitting an error." OFF)

llvm/cmake/modules/HandleLLVMOptions.cmake

if( LLVM_ENABLE_ASSERTIONS )
   endif()
 endif()
 
 if(LLVM_ENABLE_EXPENSIVE_CHECKS)
   add_definitions(-DEXPENSIVE_CHECKS)
   add_definitions(-D_GLIBCXX_DEBUG)
 endif()
 
+if (LLVM_ENABLE_STRICT_IMPLICIT_CONVERSION_TYPESIZE)
+  add_definitions(-DSTRICT_IMPLICIT_CONVERSION_TYPESIZE)
+endif()
+
 string(TOUPPER "${LLVM_ABI_BREAKING_CHECKS}" uppercase_LLVM_ABI_BREAKING_CHECKS)
 
 if( uppercase_LLVM_ABI_BREAKING_CHECKS STREQUAL "WITH_ASSERTS" )
   if( LLVM_ENABLE_ASSERTIONS )
     set( LLVM_ENABLE_ABI_BREAKING_CHECKS 1 )
   endif()
 elseif( uppercase_LLVM_ABI_BREAKING_CHECKS STREQUAL "FORCE_ON" )
   set( LLVM_ENABLE_ABI_BREAKING_CHECKS 1 )

llvm/include/llvm/Support/TypeSize.h

public:
 
   // Returns true if the number of bits is a multiple of an 8-bit byte.
   bool isByteSized() const {
     return (MinSize & 7) == 0;
   }
 
   // Casts to a uint64_t if this is a fixed-width size.
   //
-  // NOTE: This interface is obsolete and will be removed in a future version
-  // of LLVM in favour of calling getFixedSize() directly.
-  operator uint64_t() const {
+  // This interface is deprecated and will be removed in a future version
+  // of LLVM in favour of upgrading uses that rely on this implicit conversion
+  // to uint64_t. Calls to functions that return a TypeSize should use the
+  // proper interfaces to TypeSize.
+  // In practice this is mostly calls to MVT/EVT::getSizeInBits().
+  //
+  // To determine how to upgrade the code:
+  //
+  //   if (<algorithm works for both scalable and fixed-width vectors>)
+  //     use getKnownMinSize()
+  //   else if (<algorithm works only for fixed-width vectors>) {
+  //     if <algorithm can be adapted for both scalable and fixed-width vectors>
+  //       update the algorithm and use getKnownMinSize()
+  //     else
+  //       bail out early for scalable vectors and use getFixedSize()
+  //   }
+  LLVM_ATTRIBUTE_DEPRECATED(operator uint64_t() const,
+                            "Use explicit interfaces on TypeSize instead") {
+#ifdef STRICT_IMPLICIT_CONVERSION_TYPESIZE
     return getFixedSize();
+#else
+    return getKnownMinSize();
+#endif
   }
 
   // Additional convenience operators needed to avoid ambiguous parses.
   // TODO: Make uint64_t the default operator?
   TypeSize operator*(uint64_t RHS) const {
     return { MinSize * RHS, IsScalable };
   }