From 26d811b3ecd2fa1ca3d9b41e17fb42b8c7ad03d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20B=C3=B6ck?= Date: Sun, 7 Aug 2022 11:16:38 +0200 Subject: [PATCH] [mlir] Make use of C++17 language features Now that C++17 is enabled in LLVM, a lot of the TODOs and patterns to emulate C++17 features can be eliminated. The steps I have taken were essentially: ``` git grep C++17 git grep c++17 git grep "initializer_list" ``` and address given comments and patterns. Most of the changes boiled down to just using fold expressions rather than initializer_list. While doing this I also discovered that Clang by default restricts the depth of fold expressions to 256 elements. I specifically hit this with `TestDialect` in `addOperations`. I opted to not replace it with fold expressions because of that but instead adding a comment documenting the issue. If any other functions may be called with more than 256 elements in the future we might have to revert other parts as well. I don't think this is a common occurence besides the `TestDialect` however. If need be, this could potentially be fixed via `mlir-tblgen` in the future. Differential Revision: https://reviews.llvm.org/D131323 --- mlir/examples/standalone/CMakeLists.txt | 2 +- .../IR/BufferizableOpInterface.h | 15 ++--- .../Dialect/Transform/IR/TransformDialect.td | 7 +-- .../mlir/ExecutionEngine/ExecutionEngine.h | 4 +- mlir/include/mlir/IR/BuiltinAttributes.h | 1 - mlir/include/mlir/IR/BuiltinTypes.td | 2 +- mlir/include/mlir/IR/Dialect.h | 11 ++-- mlir/include/mlir/IR/DialectRegistry.h | 3 +- mlir/include/mlir/IR/Matchers.h | 7 +-- mlir/include/mlir/IR/OpDefinition.h | 25 ++------- mlir/include/mlir/IR/PatternMatch.h | 56 +++++++------------ mlir/include/mlir/IR/StorageUniquerSupport.h | 4 +- mlir/include/mlir/Support/InterfaceSupport.h | 6 +- .../Conversion/MemRefToLLVM/MemRefToLLVM.cpp | 5 +- mlir/lib/Dialect/Linalg/IR/LinalgDialect.cpp | 3 +- .../BufferizableOpInterfaceImpl.cpp | 3 +- .../Linalg/Transforms/TilingInterfaceImpl.cpp | 3 +- .../TosaLayerwiseConstantFoldPass.cpp | 3 +- mlir/lib/IR/BuiltinTypes.cpp | 3 - 19 files changed, 55 insertions(+), 108 deletions(-) diff --git a/mlir/examples/standalone/CMakeLists.txt b/mlir/examples/standalone/CMakeLists.txt index e082d817d71867..84bce58641896f 100644 --- a/mlir/examples/standalone/CMakeLists.txt +++ b/mlir/examples/standalone/CMakeLists.txt @@ -3,7 +3,7 @@ project(standalone-dialect LANGUAGES CXX C) set(CMAKE_BUILD_WITH_INSTALL_NAME_DIR ON) -set(CMAKE_CXX_STANDARD 14 CACHE STRING "C++ standard to conform to") +set(CMAKE_CXX_STANDARD 17 CACHE STRING "C++ standard to conform to") find_package(MLIR REQUIRED CONFIG) diff --git a/mlir/include/mlir/Dialect/Bufferization/IR/BufferizableOpInterface.h b/mlir/include/mlir/Dialect/Bufferization/IR/BufferizableOpInterface.h index 5d58e44570b886..e39fb696c89170 100644 --- a/mlir/include/mlir/Dialect/Bufferization/IR/BufferizableOpInterface.h +++ b/mlir/include/mlir/Dialect/Bufferization/IR/BufferizableOpInterface.h @@ -52,10 +52,8 @@ class OpFilter { template void allowDialect() { // The following expands a call to allowDialectImpl for each dialect - // in 'DialectTs'. This magic is necessary due to a limitation in the places - // that a parameter pack can be expanded in c++11. - // FIXME: In c++17 this can be simplified by using 'fold expressions'. - (void)std::initializer_list{0, (allowDialectImpl(), 0)...}; + // in 'DialectTs'. + (allowDialectImpl(), ...); } /// Deny the given dialects. @@ -63,8 +61,7 @@ class OpFilter { /// This function adds one or multiple DENY entries. template void denyDialect() { - // FIXME: In c++17 this can be simplified by using 'fold expressions'. - (void)std::initializer_list{0, (denyDialectImpl(), 0)...}; + (denyDialectImpl(), ...); } /// Allow the given dialect. @@ -82,8 +79,7 @@ class OpFilter { /// This function adds one or multiple ALLOW entries. template void allowOperation() { - // FIXME: In c++17 this can be simplified by using 'fold expressions'. - (void)std::initializer_list{0, (allowOperationImpl(), 0)...}; + (allowOperationImpl(), ...); } /// Deny the given ops. @@ -91,8 +87,7 @@ class OpFilter { /// This function adds one or multiple DENY entries. template void denyOperation() { - // FIXME: In c++17 this can be simplified by using 'fold expressions'. - (void)std::initializer_list{0, (denyOperationImpl(), 0)...}; + (denyOperationImpl(), ...); } /// Allow the given op. diff --git a/mlir/include/mlir/Dialect/Transform/IR/TransformDialect.td b/mlir/include/mlir/Dialect/Transform/IR/TransformDialect.td index f11e3da6f93d34..bf1a5224a0400a 100644 --- a/mlir/include/mlir/Dialect/Transform/IR/TransformDialect.td +++ b/mlir/include/mlir/Dialect/Transform/IR/TransformDialect.td @@ -334,13 +334,10 @@ def Transform_Dialect : Dialect { /// dialect. Checks that they implement the required interfaces. template void addOperationsChecked() { - (void)std::initializer_list{(addOperationIfNotRegistered(), - 0)...}; + (addOperationIfNotRegistered(),...); #ifndef NDEBUG - (void)std::initializer_list{ - (detail::checkImplementsTransformInterface(getContext()), - 0)...}; + (detail::checkImplementsTransformInterface(getContext()),...); #endif // NDEBUG } diff --git a/mlir/include/mlir/ExecutionEngine/ExecutionEngine.h b/mlir/include/mlir/ExecutionEngine/ExecutionEngine.h index 190b6649a95f92..7d4a708b746141 100644 --- a/mlir/include/mlir/ExecutionEngine/ExecutionEngine.h +++ b/mlir/include/mlir/ExecutionEngine/ExecutionEngine.h @@ -172,9 +172,7 @@ class ExecutionEngine { llvm::SmallVector argsArray; // Pack every arguments in an array of pointers. Delegate the packing to a // trait so that it can be overridden per argument type. - // TODO: replace with a fold expression when migrating to C++17. - int dummy[] = {0, ((void)Argument::pack(argsArray, args), 0)...}; - (void)dummy; + (Argument::pack(argsArray, args), ...); return invokePacked(adapterName, argsArray); } diff --git a/mlir/include/mlir/IR/BuiltinAttributes.h b/mlir/include/mlir/IR/BuiltinAttributes.h index d73d087597f71c..5a11a757f4ec7c 100644 --- a/mlir/include/mlir/IR/BuiltinAttributes.h +++ b/mlir/include/mlir/IR/BuiltinAttributes.h @@ -84,7 +84,6 @@ class DenseElementsAttr : public Attribute { /// Type trait used to check if the given type T is a potentially valid C++ /// floating point type that can be used to access the underlying element /// types of a DenseElementsAttr. - // TODO: Use std::disjunction when C++17 is supported. template struct is_valid_cpp_fp_type { /// The type is a valid floating point type if it is a builtin floating diff --git a/mlir/include/mlir/IR/BuiltinTypes.td b/mlir/include/mlir/IR/BuiltinTypes.td index 41336b1ea02c96..85f6b598c8e7da 100644 --- a/mlir/include/mlir/IR/BuiltinTypes.td +++ b/mlir/include/mlir/IR/BuiltinTypes.td @@ -262,7 +262,7 @@ def Builtin_Integer : Builtin_Type<"Integer"> { /// Integer representation maximal bitwidth. /// Note: This is aligned with the maximum width of llvm::IntegerType. - static constexpr unsigned kMaxWidth = (1 << 24) - 1; + static constexpr inline unsigned kMaxWidth = (1 << 24) - 1; }]; } diff --git a/mlir/include/mlir/IR/Dialect.h b/mlir/include/mlir/IR/Dialect.h index 00dd621e6f54fb..c3ae2c81b2b125 100644 --- a/mlir/include/mlir/IR/Dialect.h +++ b/mlir/include/mlir/IR/Dialect.h @@ -186,8 +186,7 @@ class Dialect { /// Register a set of dialect interfaces with this dialect instance. template void addInterfaces() { - (void)std::initializer_list{ - 0, (addInterface(std::make_unique(this)), 0)...}; + (addInterface(std::make_unique(this)), ...); } template InterfaceT &addInterface(Args &&...args) { @@ -210,6 +209,10 @@ class Dialect { /// template void addOperations() { + // This initializer_list argument pack expansion is essentially equal to + // using a fold expression with a comma operator. Clang however, refuses + // to compile a fold expression with a depth of more than 256 by default. + // There seem to be no such limitations for initializer_list. (void)std::initializer_list{ 0, (RegisteredOperationName::insert(*this), 0)...}; } @@ -217,7 +220,7 @@ class Dialect { /// Register a set of type classes with this dialect. template void addTypes() { - (void)std::initializer_list{0, (addType(), 0)...}; + (addType(), ...); } /// Register a type instance with this dialect. @@ -228,7 +231,7 @@ class Dialect { /// Register a set of attribute classes with this dialect. template void addAttributes() { - (void)std::initializer_list{0, (addAttribute(), 0)...}; + (addAttribute(), ...); } /// Register an attribute instance with this dialect. diff --git a/mlir/include/mlir/IR/DialectRegistry.h b/mlir/include/mlir/IR/DialectRegistry.h index 5e55fab3510e0a..5d2b1c80d6b477 100644 --- a/mlir/include/mlir/IR/DialectRegistry.h +++ b/mlir/include/mlir/IR/DialectRegistry.h @@ -175,8 +175,7 @@ class DialectRegistry { /// Add the given extensions to the registry. template void addExtensions() { - (void)std::initializer_list{ - (addExtension(std::make_unique()), 0)...}; + (addExtension(std::make_unique()), ...); } /// Add an extension function that requires the given dialects. diff --git a/mlir/include/mlir/IR/Matchers.h b/mlir/include/mlir/IR/Matchers.h index cc879cfc80dc41..fc7619b6206579 100644 --- a/mlir/include/mlir/IR/Matchers.h +++ b/mlir/include/mlir/IR/Matchers.h @@ -224,10 +224,9 @@ struct PatternMatcherValue { template constexpr void enumerateImpl(TupleT &&tuple, CallbackT &&callback, std::index_sequence) { - (void)std::initializer_list{ - 0, - (callback(std::integral_constant{}, std::get(tuple)), - 0)...}; + + (callback(std::integral_constant{}, std::get(tuple)), + ...); } template diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h index 27148efa0308ee..a825396d414c18 100644 --- a/mlir/include/mlir/IR/OpDefinition.h +++ b/mlir/include/mlir/IR/OpDefinition.h @@ -1492,12 +1492,10 @@ using has_fold_trait = template using detect_has_fold_trait = llvm::is_detected; /// Trait to check if T provides any `foldTrait` method. -/// NOTE: This should use std::disjunction when C++17 is available. template using detect_has_any_fold_trait = - std::conditional_t::value), - detect_has_fold_trait, - detect_has_single_result_fold_trait>; + std::disjunction, + detect_has_single_result_fold_trait>; /// Returns the result of folding a trait that implements a `foldTrait` function /// that is specialized for operations that have a single result. @@ -1543,10 +1541,7 @@ foldTrait(Operation *, ArrayRef, SmallVectorImpl &) { template static LogicalResult foldTraits(Operation *op, ArrayRef operands, SmallVectorImpl &results) { - bool anyFolded = false; - (void)std::initializer_list{ - (anyFolded |= succeeded(foldTrait(op, operands, results)), 0)...}; - return success(anyFolded); + return success((succeeded(foldTrait(op, operands, results)) | ...)); } //===----------------------------------------------------------------------===// @@ -1581,10 +1576,7 @@ verifyTrait(Operation *) { /// Given a set of traits, return the result of verifying the given operation. template LogicalResult verifyTraits(Operation *op) { - LogicalResult result = success(); - (void)std::initializer_list{ - (result = succeeded(result) ? verifyTrait(op) : failure(), 0)...}; - return result; + return success((succeeded(verifyTrait(op)) && ...)); } /// Verify the given trait if it provides a region verifier. @@ -1604,12 +1596,7 @@ verifyRegionTrait(Operation *) { /// given operation. template LogicalResult verifyRegionTraits(Operation *op) { - (void)op; - LogicalResult result = success(); - (void)std::initializer_list{ - (result = succeeded(result) ? verifyRegionTrait(op) : failure(), - 0)...}; - return result; + return success((succeeded(verifyRegionTrait(op)) && ...)); } } // namespace op_definition_impl @@ -1695,7 +1682,7 @@ class Op : public OpState, public Traits... { llvm::report_fatal_error( "Attempting to attach an interface to an unregistered operation " + ConcreteType::getOperationName() + "."); - (void)std::initializer_list{(checkInterfaceTarget(), 0)...}; + (checkInterfaceTarget(), ...); info->attachInterface(); } diff --git a/mlir/include/mlir/IR/PatternMatch.h b/mlir/include/mlir/IR/PatternMatch.h index 12bf196bb58e55..200eae8c3b71cf 100644 --- a/mlir/include/mlir/IR/PatternMatch.h +++ b/mlir/include/mlir/IR/PatternMatch.h @@ -1079,15 +1079,10 @@ LogicalResult verifyAsArgs(PatternRewriter &rewriter, ArrayRef values, auto errorFn = [&](const Twine &msg) { return rewriter.notifyMatchFailure(rewriter.getUnknownLoc(), msg); }; - LogicalResult result = success(); - (void)std::initializer_list{ - (result = - succeeded(result) - ? ProcessPDLValue>:: - verifyAsArg(errorFn, values[I], I) - : failure(), - 0)...}; - return result; + return success( + (succeeded(ProcessPDLValue>:: + verifyAsArg(errorFn, values[I], I)) && + ...)); } /// Assert that the given PDLValues match the constraints defined by the @@ -1102,10 +1097,10 @@ void assertArgs(PatternRewriter &rewriter, ArrayRef values, auto errorFn = [&](const Twine &msg) -> LogicalResult { llvm::report_fatal_error(msg); }; - (void)std::initializer_list{ - (assert(succeeded(ProcessPDLValue>::verifyAsArg(errorFn, values[I], I))), - 0)...}; + (assert(succeeded( + ProcessPDLValue>::verifyAsArg( + errorFn, values[I], I))), + ...); #endif } @@ -1134,10 +1129,7 @@ template static void processResults(PatternRewriter &rewriter, PDLResultList &results, std::tuple &&tuple) { auto applyFn = [&](auto &&...args) { - // TODO: Use proper fold expressions when we have C++17. For now we use a - // bogus std::initializer_list to work around C++14 limitations. - (void)std::initializer_list{ - (processResults(rewriter, results, std::move(args)), 0)...}; + (processResults(rewriter, results, std::move(args)), ...); }; llvm::apply_tuple(applyFn, std::move(tuple)); } @@ -1412,14 +1404,10 @@ class RewritePatternSet { typename = std::enable_if_t> RewritePatternSet &add(ConstructorArg &&arg, ConstructorArgs &&...args) { // The following expands a call to emplace_back for each of the pattern - // types 'Ts'. This magic is necessary due to a limitation in the places - // that a parameter pack can be expanded in c++11. - // FIXME: In c++17 this can be simplified by using 'fold expressions'. - (void)std::initializer_list{ - 0, (addImpl(/*debugLabels=*/llvm::None, - std::forward(arg), - std::forward(args)...), - 0)...}; + // types 'Ts'. + (addImpl(/*debugLabels=*/llvm::None, std::forward(arg), + std::forward(args)...), + ...); return *this; } /// An overload of the above `add` method that allows for attaching a set @@ -1433,11 +1421,8 @@ class RewritePatternSet { ConstructorArg &&arg, ConstructorArgs &&...args) { // The following expands a call to emplace_back for each of the pattern - // types 'Ts'. This magic is necessary due to a limitation in the places - // that a parameter pack can be expanded in c++11. - // FIXME: In c++17 this can be simplified by using 'fold expressions'. - (void)std::initializer_list{ - 0, (addImpl(debugLabels, arg, args...), 0)...}; + // types 'Ts'. + (addImpl(debugLabels, arg, args...), ...); return *this; } @@ -1445,7 +1430,7 @@ class RewritePatternSet { /// `this` for chaining insertions. template RewritePatternSet &add() { - (void)std::initializer_list{0, (addImpl(), 0)...}; + (addImpl(), ...); return *this; } @@ -1498,11 +1483,8 @@ class RewritePatternSet { typename = std::enable_if_t> RewritePatternSet &insert(ConstructorArg &&arg, ConstructorArgs &&...args) { // The following expands a call to emplace_back for each of the pattern - // types 'Ts'. This magic is necessary due to a limitation in the places - // that a parameter pack can be expanded in c++11. - // FIXME: In c++17 this can be simplified by using 'fold expressions'. - (void)std::initializer_list{ - 0, (addImpl(/*debugLabels=*/llvm::None, arg, args...), 0)...}; + // types 'Ts'. + (addImpl(/*debugLabels=*/llvm::None, arg, args...), ...); return *this; } @@ -1510,7 +1492,7 @@ class RewritePatternSet { /// `this` for chaining insertions. template RewritePatternSet &insert() { - (void)std::initializer_list{0, (addImpl(), 0)...}; + (addImpl(), ...); return *this; } diff --git a/mlir/include/mlir/IR/StorageUniquerSupport.h b/mlir/include/mlir/IR/StorageUniquerSupport.h index 7e17ba4fd7cd87..074764caf33b1f 100644 --- a/mlir/include/mlir/IR/StorageUniquerSupport.h +++ b/mlir/include/mlir/IR/StorageUniquerSupport.h @@ -138,8 +138,8 @@ class StorageUserBase : public BaseT, public Traits... { if (!abstract) llvm::report_fatal_error("Registering an interface for an attribute/type " "that is not itself registered."); - (void)std::initializer_list{ - (checkInterfaceTarget(), 0)...}; + + (checkInterfaceTarget(), ...); abstract->interfaceMap.template insert(); } diff --git a/mlir/include/mlir/Support/InterfaceSupport.h b/mlir/include/mlir/Support/InterfaceSupport.h index 53ace04453ac3e..5a56682f3bc031 100644 --- a/mlir/include/mlir/Support/InterfaceSupport.h +++ b/mlir/include/mlir/Support/InterfaceSupport.h @@ -191,16 +191,14 @@ class InterfaceMap { /// do not represent interfaces are not added to the interface map. template static InterfaceMap get() { - // TODO: Use constexpr if here in C++17. constexpr size_t numInterfaces = num_interface_types_t::value; - if (numInterfaces == 0) + if constexpr (numInterfaces == 0) return InterfaceMap(); std::array, numInterfaces> elements; std::pair *elementIt = elements.data(); (void)elementIt; - (void)std::initializer_list{ - 0, (addModelAndUpdateIterator(elementIt), 0)...}; + (addModelAndUpdateIterator(elementIt), ...); return InterfaceMap(elements); } diff --git a/mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp b/mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp index 18747df79e47b1..e3e32fa590b516 100644 --- a/mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp +++ b/mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp @@ -178,15 +178,12 @@ struct AlignedAllocOpLowering : public AllocLikeOpLLVMLowering { } /// The minimum alignment to use with aligned_alloc (has to be a power of 2). - static constexpr uint64_t kMinAlignedAllocAlignment = 16UL; + static inline constexpr uint64_t kMinAlignedAllocAlignment = 16UL; /// Default layout to use in absence of the corresponding analysis. DataLayout defaultLayout; }; -// Out of line definition, required till C++17. -constexpr uint64_t AlignedAllocOpLowering::kMinAlignedAllocAlignment; - struct AllocaOpLowering : public AllocLikeOpLLVMLowering { AllocaOpLowering(LLVMTypeConverter &converter) : AllocLikeOpLLVMLowering(memref::AllocaOp::getOperationName(), diff --git a/mlir/lib/Dialect/Linalg/IR/LinalgDialect.cpp b/mlir/lib/Dialect/Linalg/IR/LinalgDialect.cpp index 6d30f049d3d32b..6865b68c1a7707 100644 --- a/mlir/lib/Dialect/Linalg/IR/LinalgDialect.cpp +++ b/mlir/lib/Dialect/Linalg/IR/LinalgDialect.cpp @@ -96,8 +96,7 @@ void addNamedOpBuilderImpl( template void addNamedOpBuilders( llvm::StringMap &map) { - (void)std::initializer_list{0, - (addNamedOpBuilderImpl(map), 0)...}; + (addNamedOpBuilderImpl(map), ...); } void mlir::linalg::LinalgDialect::initialize() { diff --git a/mlir/lib/Dialect/Linalg/Transforms/BufferizableOpInterfaceImpl.cpp b/mlir/lib/Dialect/Linalg/Transforms/BufferizableOpInterfaceImpl.cpp index a904d7c2e1d6da..fce193eaf7c95c 100644 --- a/mlir/lib/Dialect/Linalg/Transforms/BufferizableOpInterfaceImpl.cpp +++ b/mlir/lib/Dialect/Linalg/Transforms/BufferizableOpInterfaceImpl.cpp @@ -137,8 +137,7 @@ struct LinalgOpInterface template struct LinalgOpInterfaceHelper { static void registerOpInterface(MLIRContext *ctx) { - (void)std::initializer_list{ - 0, (Ops::template attachInterface>(*ctx), 0)...}; + (Ops::template attachInterface>(*ctx), ...); } }; } // namespace diff --git a/mlir/lib/Dialect/Linalg/Transforms/TilingInterfaceImpl.cpp b/mlir/lib/Dialect/Linalg/Transforms/TilingInterfaceImpl.cpp index 9b4bd8685bbbd5..58323db48bedd0 100644 --- a/mlir/lib/Dialect/Linalg/Transforms/TilingInterfaceImpl.cpp +++ b/mlir/lib/Dialect/Linalg/Transforms/TilingInterfaceImpl.cpp @@ -263,8 +263,7 @@ static void registerOne(MLIRContext *ctx) { /// Variadic helper function. template static void registerAll(MLIRContext *ctx) { - // FIXME: In c++17 this can be simplified by using 'fold expressions'. - (void)std::initializer_list{0, (registerOne(ctx), 0)...}; + (registerOne(ctx), ...); } #define GET_OP_LIST diff --git a/mlir/lib/Dialect/Tosa/Transforms/TosaLayerwiseConstantFoldPass.cpp b/mlir/lib/Dialect/Tosa/Transforms/TosaLayerwiseConstantFoldPass.cpp index 7814b91dd6f347..aed8ff5302e516 100644 --- a/mlir/lib/Dialect/Tosa/Transforms/TosaLayerwiseConstantFoldPass.cpp +++ b/mlir/lib/Dialect/Tosa/Transforms/TosaLayerwiseConstantFoldPass.cpp @@ -23,8 +23,7 @@ namespace { template void addOpsCanonicalizations(MLIRContext *ctx, RewritePatternSet &patterns) { - (void)std::initializer_list{ - 0, (Args::getCanonicalizationPatterns(patterns, ctx), 0)...}; + (Args::getCanonicalizationPatterns(patterns, ctx), ...); } void populateTosaOpsCanonicalizationPatterns(MLIRContext *ctx, diff --git a/mlir/lib/IR/BuiltinTypes.cpp b/mlir/lib/IR/BuiltinTypes.cpp index bffd4188929cf3..d122d9c974f840 100644 --- a/mlir/lib/IR/BuiltinTypes.cpp +++ b/mlir/lib/IR/BuiltinTypes.cpp @@ -60,9 +60,6 @@ LogicalResult ComplexType::verify(function_ref emitError, // Integer Type //===----------------------------------------------------------------------===// -// static constexpr must have a definition (until in C++17 and inline variable). -constexpr unsigned IntegerType::kMaxWidth; - /// Verify the construction of an integer type. LogicalResult IntegerType::verify(function_ref emitError, unsigned width,