diff options
author | Sander de Smalen <sander.desmalen@arm.com> | 2020-03-13 09:13:34 +0000 |
---|---|---|
committer | Sander de Smalen <sander.desmalen@arm.com> | 2020-03-15 13:48:49 +0000 |
commit | 8105935d3aa98089f24b356ae782771d024d9174 (patch) | |
tree | df91cfdcd9d7b0cb3e4d257afa689da187a866bf | |
parent | 564180429818dd48f2fab970fdb42d172ebd2a5f (diff) |
[TypeSize] Allow returning scalable size in implicit conversion to uint64_t
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.
Reviewers: efriedma, huntergr, rovka, ctetreau, rengolin
Reviewed By: efriedma
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D75297
-rw-r--r-- | llvm/CMakeLists.txt | 11 | ||||
-rw-r--r-- | llvm/cmake/modules/HandleLLVMOptions.cmake | 4 | ||||
-rw-r--r-- | llvm/include/llvm/Support/TypeSize.h | 28 | ||||
-rw-r--r-- | llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp | 5 |
4 files changed, 43 insertions, 5 deletions
diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt index 23f36d5039a7..3bb037b803de 100644 --- a/llvm/CMakeLists.txt +++ b/llvm/CMakeLists.txt @@ -410,6 +410,17 @@ 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.") diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake b/llvm/cmake/modules/HandleLLVMOptions.cmake index df231eb3308c..70ad34a41bde 100644 --- a/llvm/cmake/modules/HandleLLVMOptions.cmake +++ b/llvm/cmake/modules/HandleLLVMOptions.cmake @@ -95,6 +95,10 @@ if(LLVM_ENABLE_EXPENSIVE_CHECKS) endif() 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" ) diff --git a/llvm/include/llvm/Support/TypeSize.h b/llvm/include/llvm/Support/TypeSize.h index 253190c08a04..d80031720422 100644 --- a/llvm/include/llvm/Support/TypeSize.h +++ b/llvm/include/llvm/Support/TypeSize.h @@ -15,6 +15,8 @@ #ifndef LLVM_SUPPORT_TYPESIZE_H #define LLVM_SUPPORT_TYPESIZE_H +#include "llvm/Support/WithColor.h" + #include <cstdint> #include <cassert> @@ -146,10 +148,32 @@ public: // 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. + // 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() + // } operator uint64_t() const { +#ifdef STRICT_IMPLICIT_CONVERSION_TYPESIZE return getFixedSize(); +#else + if (isScalable()) + WithColor::warning() << "Compiler has made implicit assumption that " + "TypeSize is not scalable. This may or may not " + "lead to broken code.\n"; + return getKnownMinSize(); +#endif } // Additional convenience operators needed to avoid ambiguous parses. diff --git a/llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp b/llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp index 1f14eb3a6fad..06f2ff39844d 100644 --- a/llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp +++ b/llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp @@ -14,6 +14,7 @@ #include "AMDGPULibFunc.h" #include <llvm/ADT/SmallString.h> #include <llvm/ADT/SmallVector.h> +#include "llvm/ADT/StringExtras.h" #include <llvm/ADT/StringSwitch.h> #include "llvm/IR/Attributes.h" #include "llvm/IR/DerivedTypes.h" @@ -479,8 +480,6 @@ static bool eatTerm(StringRef& mangledName, const char (&str)[N]) { return false; } -static inline bool isDigit(char c) { return c >= '0' && c <= '9'; } - static int eatNumber(StringRef& s) { size_t const savedSize = s.size(); int n = 0; @@ -605,7 +604,7 @@ bool ItaniumParamParser::parseItaniumParam(StringRef& param, // parse type char const TC = param.front(); - if (::isDigit(TC)) { + if (isDigit(TC)) { res.ArgType = StringSwitch<AMDGPULibFunc::EType> (eatLengthPrefixedName(param)) .Case("ocl_image1darray" , AMDGPULibFunc::IMG1DA) |