Skip to content

Commit 8105935

Browse files
committed
[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
1 parent 5641804 commit 8105935

File tree

4 files changed

+43
-5
lines changed

4 files changed

+43
-5
lines changed

llvm/CMakeLists.txt

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,17 @@ endif()
410410

411411
option(LLVM_ENABLE_EXPENSIVE_CHECKS "Enable expensive checks" OFF)
412412

413+
# While adding scalable vector support to LLVM, we temporarily want to
414+
# allow an implicit conversion of TypeSize to uint64_t. This CMake flag
415+
# enables a more strict conversion where it asserts that the type is not
416+
# a scalable vector type.
417+
#
418+
# Enabling this flag makes it easier to find cases where the compiler makes
419+
# assumptions on the size being 'fixed size', when building tests for
420+
# SVE/SVE2 or other scalable vector architectures.
421+
option(LLVM_ENABLE_STRICT_IMPLICIT_CONVERSION_TYPESIZE
422+
"Enable assertions that type is not scalable in implicit conversion from TypeSize to uint64_t" OFF)
423+
413424
set(LLVM_ABI_BREAKING_CHECKS "WITH_ASSERTS" CACHE STRING
414425
"Enable abi-breaking checks. Can be WITH_ASSERTS, FORCE_ON or FORCE_OFF.")
415426

llvm/cmake/modules/HandleLLVMOptions.cmake

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,10 @@ if(LLVM_ENABLE_EXPENSIVE_CHECKS)
9595
endif()
9696
endif()
9797

98+
if (LLVM_ENABLE_STRICT_IMPLICIT_CONVERSION_TYPESIZE)
99+
add_definitions(-DSTRICT_IMPLICIT_CONVERSION_TYPESIZE)
100+
endif()
101+
98102
string(TOUPPER "${LLVM_ABI_BREAKING_CHECKS}" uppercase_LLVM_ABI_BREAKING_CHECKS)
99103

100104
if( uppercase_LLVM_ABI_BREAKING_CHECKS STREQUAL "WITH_ASSERTS" )

llvm/include/llvm/Support/TypeSize.h

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
#ifndef LLVM_SUPPORT_TYPESIZE_H
1616
#define LLVM_SUPPORT_TYPESIZE_H
1717

18+
#include "llvm/Support/WithColor.h"
19+
1820
#include <cstdint>
1921
#include <cassert>
2022

@@ -146,10 +148,32 @@ class TypeSize {
146148

147149
// Casts to a uint64_t if this is a fixed-width size.
148150
//
149-
// NOTE: This interface is obsolete and will be removed in a future version
150-
// of LLVM in favour of calling getFixedSize() directly.
151+
// This interface is deprecated and will be removed in a future version
152+
// of LLVM in favour of upgrading uses that rely on this implicit conversion
153+
// to uint64_t. Calls to functions that return a TypeSize should use the
154+
// proper interfaces to TypeSize.
155+
// In practice this is mostly calls to MVT/EVT::getSizeInBits().
156+
//
157+
// To determine how to upgrade the code:
158+
//
159+
// if (<algorithm works for both scalable and fixed-width vectors>)
160+
// use getKnownMinSize()
161+
// else if (<algorithm works only for fixed-width vectors>) {
162+
// if <algorithm can be adapted for both scalable and fixed-width vectors>
163+
// update the algorithm and use getKnownMinSize()
164+
// else
165+
// bail out early for scalable vectors and use getFixedSize()
166+
// }
151167
operator uint64_t() const {
168+
#ifdef STRICT_IMPLICIT_CONVERSION_TYPESIZE
152169
return getFixedSize();
170+
#else
171+
if (isScalable())
172+
WithColor::warning() << "Compiler has made implicit assumption that "
173+
"TypeSize is not scalable. This may or may not "
174+
"lead to broken code.\n";
175+
return getKnownMinSize();
176+
#endif
153177
}
154178

155179
// Additional convenience operators needed to avoid ambiguous parses.

llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "AMDGPULibFunc.h"
1515
#include <llvm/ADT/SmallString.h>
1616
#include <llvm/ADT/SmallVector.h>
17+
#include "llvm/ADT/StringExtras.h"
1718
#include <llvm/ADT/StringSwitch.h>
1819
#include "llvm/IR/Attributes.h"
1920
#include "llvm/IR/DerivedTypes.h"
@@ -479,8 +480,6 @@ static bool eatTerm(StringRef& mangledName, const char (&str)[N]) {
479480
return false;
480481
}
481482

482-
static inline bool isDigit(char c) { return c >= '0' && c <= '9'; }
483-
484483
static int eatNumber(StringRef& s) {
485484
size_t const savedSize = s.size();
486485
int n = 0;
@@ -605,7 +604,7 @@ bool ItaniumParamParser::parseItaniumParam(StringRef& param,
605604

606605
// parse type
607606
char const TC = param.front();
608-
if (::isDigit(TC)) {
607+
if (isDigit(TC)) {
609608
res.ArgType = StringSwitch<AMDGPULibFunc::EType>
610609
(eatLengthPrefixedName(param))
611610
.Case("ocl_image1darray" , AMDGPULibFunc::IMG1DA)

0 commit comments

Comments
 (0)