Skip to content

Commit

Permalink
[mlir] Make use of C++17 language features
Browse files Browse the repository at this point in the history
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<int>"
```
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
  • Loading branch information
zero9178 committed Aug 7, 2022
1 parent f0f1bca commit 26d811b
Show file tree
Hide file tree
Showing 19 changed files with 55 additions and 108 deletions.
2 changes: 1 addition & 1 deletion mlir/examples/standalone/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,19 +52,16 @@ class OpFilter {
template <typename... DialectTs>
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<int>{0, (allowDialectImpl<DialectTs>(), 0)...};
// in 'DialectTs'.
(allowDialectImpl<DialectTs>(), ...);
}

/// Deny the given dialects.
///
/// This function adds one or multiple DENY entries.
template <typename... DialectTs>
void denyDialect() {
// FIXME: In c++17 this can be simplified by using 'fold expressions'.
(void)std::initializer_list<int>{0, (denyDialectImpl<DialectTs>(), 0)...};
(denyDialectImpl<DialectTs>(), ...);
}

/// Allow the given dialect.
Expand All @@ -82,17 +79,15 @@ class OpFilter {
/// This function adds one or multiple ALLOW entries.
template <typename... OpTys>
void allowOperation() {
// FIXME: In c++17 this can be simplified by using 'fold expressions'.
(void)std::initializer_list<int>{0, (allowOperationImpl<OpTys>(), 0)...};
(allowOperationImpl<OpTys>(), ...);
}

/// Deny the given ops.
///
/// This function adds one or multiple DENY entries.
template <typename... OpTys>
void denyOperation() {
// FIXME: In c++17 this can be simplified by using 'fold expressions'.
(void)std::initializer_list<int>{0, (denyOperationImpl<OpTys>(), 0)...};
(denyOperationImpl<OpTys>(), ...);
}

/// Allow the given op.
Expand Down
7 changes: 2 additions & 5 deletions mlir/include/mlir/Dialect/Transform/IR/TransformDialect.td
Original file line number Diff line number Diff line change
Expand Up @@ -334,13 +334,10 @@ def Transform_Dialect : Dialect {
/// dialect. Checks that they implement the required interfaces.
template <typename... OpTys>
void addOperationsChecked() {
(void)std::initializer_list<int>{(addOperationIfNotRegistered<OpTys>(),
0)...};
(addOperationIfNotRegistered<OpTys>(),...);

#ifndef NDEBUG
(void)std::initializer_list<int>{
(detail::checkImplementsTransformInterface<OpTys>(getContext()),
0)...};
(detail::checkImplementsTransformInterface<OpTys>(getContext()),...);
#endif // NDEBUG
}

Expand Down
4 changes: 1 addition & 3 deletions mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,7 @@ class ExecutionEngine {
llvm::SmallVector<void *> 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<Args>::pack(argsArray, args), 0)...};
(void)dummy;
(Argument<Args>::pack(argsArray, args), ...);
return invokePacked(adapterName, argsArray);
}

Expand Down
1 change: 0 additions & 1 deletion mlir/include/mlir/IR/BuiltinAttributes.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 <typename T>
struct is_valid_cpp_fp_type {
/// The type is a valid floating point type if it is a builtin floating
Expand Down
2 changes: 1 addition & 1 deletion mlir/include/mlir/IR/BuiltinTypes.td
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}];
}

Expand Down
11 changes: 7 additions & 4 deletions mlir/include/mlir/IR/Dialect.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,7 @@ class Dialect {
/// Register a set of dialect interfaces with this dialect instance.
template <typename... Args>
void addInterfaces() {
(void)std::initializer_list<int>{
0, (addInterface(std::make_unique<Args>(this)), 0)...};
(addInterface(std::make_unique<Args>(this)), ...);
}
template <typename InterfaceT, typename... Args>
InterfaceT &addInterface(Args &&...args) {
Expand All @@ -210,14 +209,18 @@ class Dialect {
///
template <typename... Args>
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<int>{
0, (RegisteredOperationName::insert<Args>(*this), 0)...};
}

/// Register a set of type classes with this dialect.
template <typename... Args>
void addTypes() {
(void)std::initializer_list<int>{0, (addType<Args>(), 0)...};
(addType<Args>(), ...);
}

/// Register a type instance with this dialect.
Expand All @@ -228,7 +231,7 @@ class Dialect {
/// Register a set of attribute classes with this dialect.
template <typename... Args>
void addAttributes() {
(void)std::initializer_list<int>{0, (addAttribute<Args>(), 0)...};
(addAttribute<Args>(), ...);
}

/// Register an attribute instance with this dialect.
Expand Down
3 changes: 1 addition & 2 deletions mlir/include/mlir/IR/DialectRegistry.h
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ class DialectRegistry {
/// Add the given extensions to the registry.
template <typename... ExtensionsT>
void addExtensions() {
(void)std::initializer_list<int>{
(addExtension(std::make_unique<ExtensionsT>()), 0)...};
(addExtension(std::make_unique<ExtensionsT>()), ...);
}

/// Add an extension function that requires the given dialects.
Expand Down
7 changes: 3 additions & 4 deletions mlir/include/mlir/IR/Matchers.h
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,9 @@ struct PatternMatcherValue {
template <typename TupleT, class CallbackT, std::size_t... Is>
constexpr void enumerateImpl(TupleT &&tuple, CallbackT &&callback,
std::index_sequence<Is...>) {
(void)std::initializer_list<int>{
0,
(callback(std::integral_constant<std::size_t, Is>{}, std::get<Is>(tuple)),
0)...};

(callback(std::integral_constant<std::size_t, Is>{}, std::get<Is>(tuple)),
...);
}

template <typename... Tys, typename CallbackT>
Expand Down
25 changes: 6 additions & 19 deletions mlir/include/mlir/IR/OpDefinition.h
Original file line number Diff line number Diff line change
Expand Up @@ -1492,12 +1492,10 @@ using has_fold_trait =
template <typename T>
using detect_has_fold_trait = llvm::is_detected<has_fold_trait, T>;
/// Trait to check if T provides any `foldTrait` method.
/// NOTE: This should use std::disjunction when C++17 is available.
template <typename T>
using detect_has_any_fold_trait =
std::conditional_t<bool(detect_has_fold_trait<T>::value),
detect_has_fold_trait<T>,
detect_has_single_result_fold_trait<T>>;
std::disjunction<detect_has_fold_trait<T>,
detect_has_single_result_fold_trait<T>>;

/// Returns the result of folding a trait that implements a `foldTrait` function
/// that is specialized for operations that have a single result.
Expand Down Expand Up @@ -1543,10 +1541,7 @@ foldTrait(Operation *, ArrayRef<Attribute>, SmallVectorImpl<OpFoldResult> &) {
template <typename... Ts>
static LogicalResult foldTraits(Operation *op, ArrayRef<Attribute> operands,
SmallVectorImpl<OpFoldResult> &results) {
bool anyFolded = false;
(void)std::initializer_list<int>{
(anyFolded |= succeeded(foldTrait<Ts>(op, operands, results)), 0)...};
return success(anyFolded);
return success((succeeded(foldTrait<Ts>(op, operands, results)) | ...));
}

//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -1581,10 +1576,7 @@ verifyTrait(Operation *) {
/// Given a set of traits, return the result of verifying the given operation.
template <typename... Ts>
LogicalResult verifyTraits(Operation *op) {
LogicalResult result = success();
(void)std::initializer_list<int>{
(result = succeeded(result) ? verifyTrait<Ts>(op) : failure(), 0)...};
return result;
return success((succeeded(verifyTrait<Ts>(op)) && ...));
}

/// Verify the given trait if it provides a region verifier.
Expand All @@ -1604,12 +1596,7 @@ verifyRegionTrait(Operation *) {
/// given operation.
template <typename... Ts>
LogicalResult verifyRegionTraits(Operation *op) {
(void)op;
LogicalResult result = success();
(void)std::initializer_list<int>{
(result = succeeded(result) ? verifyRegionTrait<Ts>(op) : failure(),
0)...};
return result;
return success((succeeded(verifyRegionTrait<Ts>(op)) && ...));
}
} // namespace op_definition_impl

Expand Down Expand Up @@ -1695,7 +1682,7 @@ class Op : public OpState, public Traits<ConcreteType>... {
llvm::report_fatal_error(
"Attempting to attach an interface to an unregistered operation " +
ConcreteType::getOperationName() + ".");
(void)std::initializer_list<int>{(checkInterfaceTarget<Models>(), 0)...};
(checkInterfaceTarget<Models>(), ...);
info->attachInterface<Models...>();
}

Expand Down
56 changes: 19 additions & 37 deletions mlir/include/mlir/IR/PatternMatch.h
Original file line number Diff line number Diff line change
Expand Up @@ -1079,15 +1079,10 @@ LogicalResult verifyAsArgs(PatternRewriter &rewriter, ArrayRef<PDLValue> values,
auto errorFn = [&](const Twine &msg) {
return rewriter.notifyMatchFailure(rewriter.getUnknownLoc(), msg);
};
LogicalResult result = success();
(void)std::initializer_list<int>{
(result =
succeeded(result)
? ProcessPDLValue<typename FnTraitsT::template arg_t<I + 1>>::
verifyAsArg(errorFn, values[I], I)
: failure(),
0)...};
return result;
return success(
(succeeded(ProcessPDLValue<typename FnTraitsT::template arg_t<I + 1>>::
verifyAsArg(errorFn, values[I], I)) &&
...));
}

/// Assert that the given PDLValues match the constraints defined by the
Expand All @@ -1102,10 +1097,10 @@ void assertArgs(PatternRewriter &rewriter, ArrayRef<PDLValue> values,
auto errorFn = [&](const Twine &msg) -> LogicalResult {
llvm::report_fatal_error(msg);
};
(void)std::initializer_list<int>{
(assert(succeeded(ProcessPDLValue<typename FnTraitsT::template arg_t<
I + 1>>::verifyAsArg(errorFn, values[I], I))),
0)...};
(assert(succeeded(
ProcessPDLValue<typename FnTraitsT::template arg_t<I + 1>>::verifyAsArg(
errorFn, values[I], I))),
...);
#endif
}

Expand Down Expand Up @@ -1134,10 +1129,7 @@ template <typename... Ts>
static void processResults(PatternRewriter &rewriter, PDLResultList &results,
std::tuple<Ts...> &&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<int>{
(processResults(rewriter, results, std::move(args)), 0)...};
(processResults(rewriter, results, std::move(args)), ...);
};
llvm::apply_tuple(applyFn, std::move(tuple));
}
Expand Down Expand Up @@ -1412,14 +1404,10 @@ class RewritePatternSet {
typename = std::enable_if_t<sizeof...(Ts) != 0>>
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<int>{
0, (addImpl<Ts>(/*debugLabels=*/llvm::None,
std::forward<ConstructorArg>(arg),
std::forward<ConstructorArgs>(args)...),
0)...};
// types 'Ts'.
(addImpl<Ts>(/*debugLabels=*/llvm::None, std::forward<ConstructorArg>(arg),
std::forward<ConstructorArgs>(args)...),
...);
return *this;
}
/// An overload of the above `add` method that allows for attaching a set
Expand All @@ -1433,19 +1421,16 @@ 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<int>{
0, (addImpl<Ts>(debugLabels, arg, args...), 0)...};
// types 'Ts'.
(addImpl<Ts>(debugLabels, arg, args...), ...);
return *this;
}

/// Add an instance of each of the pattern types 'Ts'. Return a reference to
/// `this` for chaining insertions.
template <typename... Ts>
RewritePatternSet &add() {
(void)std::initializer_list<int>{0, (addImpl<Ts>(), 0)...};
(addImpl<Ts>(), ...);
return *this;
}

Expand Down Expand Up @@ -1498,19 +1483,16 @@ class RewritePatternSet {
typename = std::enable_if_t<sizeof...(Ts) != 0>>
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<int>{
0, (addImpl<Ts>(/*debugLabels=*/llvm::None, arg, args...), 0)...};
// types 'Ts'.
(addImpl<Ts>(/*debugLabels=*/llvm::None, arg, args...), ...);
return *this;
}

/// Add an instance of each of the pattern types 'Ts'. Return a reference to
/// `this` for chaining insertions.
template <typename... Ts>
RewritePatternSet &insert() {
(void)std::initializer_list<int>{0, (addImpl<Ts>(), 0)...};
(addImpl<Ts>(), ...);
return *this;
}

Expand Down
4 changes: 2 additions & 2 deletions mlir/include/mlir/IR/StorageUniquerSupport.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ class StorageUserBase : public BaseT, public Traits<ConcreteT>... {
if (!abstract)
llvm::report_fatal_error("Registering an interface for an attribute/type "
"that is not itself registered.");
(void)std::initializer_list<int>{
(checkInterfaceTarget<IfaceModels>(), 0)...};

(checkInterfaceTarget<IfaceModels>(), ...);
abstract->interfaceMap.template insert<IfaceModels...>();
}

Expand Down
6 changes: 2 additions & 4 deletions mlir/include/mlir/Support/InterfaceSupport.h
Original file line number Diff line number Diff line change
Expand Up @@ -191,16 +191,14 @@ class InterfaceMap {
/// do not represent interfaces are not added to the interface map.
template <typename... Types>
static InterfaceMap get() {
// TODO: Use constexpr if here in C++17.
constexpr size_t numInterfaces = num_interface_types_t<Types...>::value;
if (numInterfaces == 0)
if constexpr (numInterfaces == 0)
return InterfaceMap();

std::array<std::pair<TypeID, void *>, numInterfaces> elements;
std::pair<TypeID, void *> *elementIt = elements.data();
(void)elementIt;
(void)std::initializer_list<int>{
0, (addModelAndUpdateIterator<Types>(elementIt), 0)...};
(addModelAndUpdateIterator<Types>(elementIt), ...);
return InterfaceMap(elements);
}

Expand Down
5 changes: 1 addition & 4 deletions mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
3 changes: 1 addition & 2 deletions mlir/lib/Dialect/Linalg/IR/LinalgDialect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,7 @@ void addNamedOpBuilderImpl(
template <typename... OpTypes>
void addNamedOpBuilders(
llvm::StringMap<LinalgDialect::RegionBuilderFunType> &map) {
(void)std::initializer_list<int>{0,
(addNamedOpBuilderImpl<OpTypes>(map), 0)...};
(addNamedOpBuilderImpl<OpTypes>(map), ...);
}

void mlir::linalg::LinalgDialect::initialize() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,7 @@ struct LinalgOpInterface
template <typename... Ops>
struct LinalgOpInterfaceHelper {
static void registerOpInterface(MLIRContext *ctx) {
(void)std::initializer_list<int>{
0, (Ops::template attachInterface<LinalgOpInterface<Ops>>(*ctx), 0)...};
(Ops::template attachInterface<LinalgOpInterface<Ops>>(*ctx), ...);
}
};
} // namespace
Expand Down
Loading

0 comments on commit 26d811b

Please sign in to comment.