[Support][Casting] Add predicates for `isa*` functions #83753

kuhar · 2024-03-04T02:08:15Z

Expose function objects that call into llvm::isa and llvm::isa_and_present, such that these type checks can be used as predicates in generic algorithms.

Before this change, llvm::isa* functions cannot be easily used without knowing both the argument type and the checked types, which leads to them being wrapped in lambdas. For example:

llvm::all_of(myTypes,
             [](auto type) { return llvm::isa<VectorType>(type); });

With this PR the example above becomes:

llvm::all_of(myTypes, llvm::IsaPred<VectorType>);

As an alternative solution, I considered redefining isa* as function objects, but I decided against doing that because it would create asymmetry with other cast functions and could break code that depends on them being actual functions.

llvmbot · 2024-03-04T02:08:47Z

@llvm/pr-subscribers-llvm-support

Author: Jakub Kuderski (kuhar)

Changes

Expose function objects that call into llvm::isa and llvm::isa_and_present, such that these type checks can be used as predicates in generic algorithms.

Before this change, llvm::isa* functions cannot be easily used without knowing both the argument type and the checked types, which leads to them being wrapped in lambdas. For example:

llvm::all_of(myTypes,
             [](auto type) { return llvm::isa&lt;VectorType&gt;(type); });

With this PR the example above becomes:

llvm::all_of(myTypes, llvm::IsaPred&lt;VectorType&gt;);

As an alternative solution, I considered redefining isa* as function objects, but I decided against doing that because it would create asymmetry with other cast functions and could break code that depends on them being actual functions.

Full diff: https://github.com/llvm/llvm-project/pull/83753.diff

2 Files Affected:

(modified) llvm/include/llvm/Support/Casting.h (+46)
(modified) llvm/unittests/Support/Casting.cpp (+14)

diff --git a/llvm/include/llvm/Support/Casting.h b/llvm/include/llvm/Support/Casting.h
index 2391c1a531a5be..4c0ae59b1aa986 100644
--- a/llvm/include/llvm/Support/Casting.h
+++ b/llvm/include/llvm/Support/Casting.h
@@ -801,6 +801,52 @@ template <class X, class Y>
   return unique_dyn_cast_or_null<X, Y>(Val);
 }
 
+//===----------------------------------------------------------------------===//
+// Isa Predicates
+//===----------------------------------------------------------------------===//
+
+/// These are wrappers over isa* function that allow them to be used in generic
+/// algorithms such as `llvm:all_of`, `llvm::none_of`, etc. This is accomplished
+/// by exposing the isa* functions through function objects with a generic
+/// function call operator.
+
+namespace detail {
+template <typename... Types> struct IsaCheckPredicate {
+  template <typename T> bool operator()(const T &Val) const {
+    return isa<Types...>(Val);
+  }
+};
+
+template <typename... Types> struct IsaAndPresentCheckPredicate {
+  template <typename T> bool operator()(const T &Val) const {
+    return isa_and_present<Types...>(Val);
+  }
+};
+} // namespace detail
+
+/// Function object wrapper for the `llvm::isa` type check. The function call
+/// operator returns true when the value can be cast to any type in `Types`.
+/// Example:
+/// ```
+/// SmallVector<Type> myTypes = ...;
+/// if (llvm::all_of(myTypes, llvm::IsaPred<VectorType>))
+///   ...
+/// ```
+template <typename... Types>
+inline constexpr detail::IsaCheckPredicate<Types...> IsaPred{};
+
+/// Function object wrapper for the `llvm::isa_and_present` type check. The
+/// function call operator returns true when the value can be cast to any type
+/// in `Types`, or if the value is not present (e.g., nullptr). Example:
+/// ```
+/// SmallVector<Type> myTypes = ...;
+/// if (llvm::all_of(myTypes, llvm::IsaAndPresnetPred<VectorType>))
+///   ...
+/// ```
+template <typename... Types>
+inline constexpr detail::IsaAndPresentCheckPredicate<Types...>
+    IsaAndPresentPred{};
+
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_CASTING_H
diff --git a/llvm/unittests/Support/Casting.cpp b/llvm/unittests/Support/Casting.cpp
index a9256548145986..2f8b0c50e1955c 100644
--- a/llvm/unittests/Support/Casting.cpp
+++ b/llvm/unittests/Support/Casting.cpp
@@ -282,6 +282,20 @@ TEST(CastingTest, dyn_cast_if_present) {
   EXPECT_FALSE(t4.hasValue);
 }
 
+TEST(CastingTest, isa_check_predicates) {
+  auto IsaFoo = IsaPred<foo>;
+  auto IsaAndPresentFoo = IsaAndPresentPred<foo>;
+  EXPECT_TRUE(IsaFoo(B1));
+  EXPECT_TRUE(IsaFoo(B2));
+  EXPECT_TRUE(IsaFoo(B3));
+  EXPECT_TRUE(IsaPred<foo>(B4));
+  EXPECT_TRUE((IsaPred<foo, bar>(B4)));
+  EXPECT_TRUE(IsaAndPresentFoo(B2));
+  EXPECT_TRUE(IsaAndPresentFoo(B4));
+  EXPECT_FALSE(IsaAndPresentPred<foo>(fub()));
+  EXPECT_FALSE((IsaAndPresentPred<foo, bar>(fub())));
+}
+
 std::unique_ptr<derived> newd() { return std::make_unique<derived>(); }
 std::unique_ptr<base> newb() { return std::make_unique<derived>(); }

zero9178

LGTM!

dwblaikie · 2024-03-04T21:09:58Z

It'd probably be an issue for some callers, but we could make the actual isa, etc into functors so we don't need to introduce new names? Something like this: https://stackoverflow.com/questions/62928396/what-is-a-niebloid (not in this case to thwart overloading, but to make it easy to pass as functors)

kuhar · 2024-03-04T22:22:19Z

It'd probably be an issue for some callers, but we could make the actual isa, etc into functors so we don't need to introduce new names? Something like this: https://stackoverflow.com/questions/62928396/what-is-a-niebloid (not in this case to thwart overloading, but to make it easy to pass as functors)

I'm worried about the side effects, given that this is probably one of the most commonly used function across the whole project and downstream projects:

Any code that explicitly specified the From template argument would silently break (the interpretation would change to another possible cast type)
This disables ADL

kuhar · 2024-03-05T14:12:58Z

WDYT @dwblaikie?

dwblaikie · 2024-03-05T17:48:08Z

It'd probably be an issue for some callers, but we could make the actual isa, etc into functors so we don't need to introduce new names? Something like this: https://stackoverflow.com/questions/62928396/what-is-a-niebloid (not in this case to thwart overloading, but to make it easy to pass as functors)

I'm worried about the side effects, given that this is probably one of the most commonly used function across the whole project and downstream projects:

While it's used a lot, I'd hope it's used enough in LLVM that we'd get a pretty good idea of breakage from LLVM, but yeah, probably some novel things in downstream users.

Any code that explicitly specified the From template argument would silently break (the interpretation would change to another possible cast type)

Hmm, not sure I follow this - I'd expect the functor to have one template parameter, so anyone explicitly specifying 2 would fail to compile at least...

This disables ADL

Yeah, that's a hard one to argue with... I'm sure there's a bunch of code out there doing unqualified isa/cast etc that only works due to ADL. Which in some ways might not be a bad thing (big discussions about ADL costs/complexities/maintenance issues except where needed/intended for things like swap) - but probably a separate discussion not worth binding to this change/direction.

As an alternative solution, I considered redefining isa* as function objects, but I decided against doing that because it would create asymmetry with other cast functions and could break code that depends on them being actual functions.

Oh, sorry I didn't read this the first time through, btw - thanks for considering/noting it.

Open to discussion here - but I'm inclined to think the ability specify the second template parameter to isa is a bug/incidental feature of isa being a function template, not a feature & one we could choose not to expose in these predicates (by only providing one template parameter, rather than the parameter pack on the outer functor class)? Like if someone wants to use a specific argument type - they should convert the argument expression themselves, rather than by specifying a second template argument and then relying on impliict conversion of the function parameter?

kuhar · 2024-03-05T18:24:26Z

Any code that explicitly specified the From template argument would silently break (the interpretation would change to another possible cast type)

Hmm, not sure I follow this - I'd expect the functor to have one template parameter, so anyone explicitly specifying 2 would fail to compile at least...

@dwblaikie Currently, isa is defined like this:

llvm-project/llvm/include/llvm/Support/Casting.h

Lines 547 to 555 in ee044d5

    
           template <typename To, typename From> 
        
           [[nodiscard]] inline bool isa(const From &Val) { 
        
             return CastInfo<To, const From>::isPossible(Val); 
        
           } 
        
           template <typename First, typename Second, typename... Rest, typename From> 
        
           [[nodiscard]] inline bool isa(const From &Val) { 
        
             return isa<First>(Val) || isa<Second, Rest...>(Val); 
        
           }

I'm worried that the meaning of &isa</*To=*/T1, /*From=*/T2> would change to &isa</*To1=*/ T1, /*To2=*/T2> in the context of something like:

auto fPtr = &isa<VectorType, Type>;
...
bool isA = (*fPtr)(ptr);

dwblaikie · 2024-03-05T20:14:16Z

Any code that explicitly specified the From template argument would silently break (the interpretation would change to another possible cast type)

Hmm, not sure I follow this - I'd expect the functor to have one template parameter, so anyone explicitly specifying 2 would fail to compile at least...

@dwblaikie Currently, isa is defined like this:

llvm-project/llvm/include/llvm/Support/Casting.h

Lines 547 to 555 in ee044d5

template <typename To, typename From>

[[nodiscard]] inline bool isa(const From &Val) {

return CastInfo<To, const From>::isPossible(Val);

}

template <typename First, typename Second, typename... Rest, typename From>

[[nodiscard]] inline bool isa(const From &Val) {

return isa<First>(Val) || isa<Second, Rest...>(Val);

}

I'm worried that the meaning of &isa</*To=*/T1, /*From=*/T2> would change to &isa</*To1=*/ T1, /*To2=*/T2> in the context of something like:
auto fPtr = &isa<VectorType, Type>;
...
bool isA = (*fPtr)(ptr);

Hmm, sorry, not really following... I don't think we'd support multiple target types... it'd have one template parameter, the target type, and specifying 2 would fail to compile. (I think the ability to specify the from type is sort of incidental/accident of the template implementation, not an intentional feature of isa/cast/etc)

but it's probably irrelevant, since, as you say, disabling ADL means functor solutions aren't likely ideal/feasible for backwards compatibility reasons.

Open to discussion here - but I'm inclined to think the ability specify the second template parameter to isa is a bug/incidental feature of isa being a function template, not a feature & one we could choose not to expose in these predicates (by only providing one template parameter, rather than the parameter pack on the outer functor class)? Like if someone wants to use a specific argument type - they should convert the argument expression themselves, rather than by specifying a second template argument and then relying on impliict conversion of the function parameter?

Still be curious about this ^ but a relatively minor issue.

kuhar · 2024-03-05T21:23:31Z

Hmm, sorry, not really following... I don't think we'd support multiple target types... it'd have one template parameter, the target type, and specifying 2 would fail to compile. (I think the ability to specify the from type is sort of incidental/accident of the template implementation, not an intentional feature of isa/cast/etc)

@dwblaikie But we do support it today. You can provide one or more target types. Some examples:

➜ grep -rn 'isa<IntegerType, FloatType>' .
./third_party/llvm-project/mlir/lib/IR/Types.cpp:118:  return llvm::isa<IntegerType, FloatType>(*this);
./third_party/llvm-project/mlir/lib/Interfaces/DataLayoutInterfaces.cpp:54:  if (isa<IntegerType, FloatType>(type))
./third_party/llvm-project/mlir/lib/Interfaces/DataLayoutInterfaces.cpp:627:    if (isa<IntegerType, FloatType>(sampleType)) {
./third_party/llvm-project/mlir/lib/Target/LLVMIR/ModuleImport.cpp:662:  return isa<IntegerType, FloatType>(type);
./compiler/src/iree/compiler/Dialect/Util/Analysis/Constant/OpOracle.cpp:51:  if (llvm::isa<IntegerType, FloatType>(t)) {
./compiler/src/iree/compiler/Codegen/Utils/GPUUtils.cpp:387:  assert((llvm::isa<IntegerType, FloatType>(input.getType())) &&
./compiler/src/iree/compiler/Codegen/SPIRV/KernelConfig.cpp:1367:    return isa<IntegerType, FloatType>(elementType) &&

➜  llvm-project git:(4df364bc93af) grep -rn 'isa<.*Type, .*Type>' . | wc -l
106

If this wasn't allowed, I'd aggrege there's no possibility for confusion.

Open to discussion here - but I'm inclined to think the ability specify the second template parameter to isa is a bug/incidental feature of isa being a function template, not a feature & one we could choose not to expose in these predicates (by only providing one template parameter, rather than the parameter pack on the outer functor class)? Like if someone wants to use a specific argument type - they should convert the argument expression themselves, rather than by specifying a second template argument and then relying on impliict conversion of the function parameter?

Still be curious about this ^ but a relatively minor issue.

I have similar feeling, but because this is more like an 'accidental bug in C++', I'd prefer to not to apply it extremely selectively to 2 functions in the whole project.

To summarize, because I've spent probably days/weeks an various llvm integration rotations, I'm sympathetic to downstream maintainers and would like to avoid potential breakage. The ADL thing + function pointer template argument behavior change would make me uncomfortable redefining a very core API. This would deserve a wider discussion / RFC, and if we want to move in this direction, we can do it incrementally: land this first as a separate API, consider replacing the main isa* functions later.

topperc · 2024-03-05T21:29:47Z

Should this replace the multiple places in SLPVectorizer.cpp that pass classof to all_of/count_if/etc?

dwblaikie · 2024-03-06T00:32:00Z

Hmm, sorry, not really following... I don't think we'd support multiple target types... it'd have one template parameter, the target type, and specifying 2 would fail to compile. (I think the ability to specify the from type is sort of incidental/accident of the template implementation, not an intentional feature of isa/cast/etc)

@dwblaikie But we do support it today. You can provide one or more target types. Some examples:
➜ grep -rn 'isa<IntegerType, FloatType>' .
./third_party/llvm-project/mlir/lib/IR/Types.cpp:118:  return llvm::isa<IntegerType, FloatType>(*this);
./third_party/llvm-project/mlir/lib/Interfaces/DataLayoutInterfaces.cpp:54:  if (isa<IntegerType, FloatType>(type))
./third_party/llvm-project/mlir/lib/Interfaces/DataLayoutInterfaces.cpp:627:    if (isa<IntegerType, FloatType>(sampleType)) {
./third_party/llvm-project/mlir/lib/Target/LLVMIR/ModuleImport.cpp:662:  return isa<IntegerType, FloatType>(type);
./compiler/src/iree/compiler/Dialect/Util/Analysis/Constant/OpOracle.cpp:51:  if (llvm::isa<IntegerType, FloatType>(t)) {
./compiler/src/iree/compiler/Codegen/Utils/GPUUtils.cpp:387:  assert((llvm::isa<IntegerType, FloatType>(input.getType())) &&
./compiler/src/iree/compiler/Codegen/SPIRV/KernelConfig.cpp:1367:    return isa<IntegerType, FloatType>(elementType) &&
➜  llvm-project git:(4df364bc93af) grep -rn 'isa<.*Type, .*Type>' . | wc -l
106
If this wasn't allowed, I'd aggrege there's no possibility for confusion.

Open to discussion here - but I'm inclined to think the ability specify the second template parameter to isa is a bug/incidental feature of isa being a function template, not a feature & one we could choose not to expose in these predicates (by only providing one template parameter, rather than the parameter pack on the outer functor class)? Like if someone wants to use a specific argument type - they should convert the argument expression themselves, rather than by specifying a second template argument and then relying on impliict conversion of the function parameter?

Still be curious about this ^ but a relatively minor issue.

I have similar feeling, but because this is more like an 'accidental bug in C++', I'd prefer to not to apply it extremely selectively to 2 functions in the whole project.

To summarize, because I've spent probably days/weeks an various llvm integration rotations, I'm sympathetic to downstream maintainers and would like to avoid potential breakage. The ADL thing + function pointer template argument behavior change would make me uncomfortable redefining a very core API. This would deserve a wider discussion / RFC, and if we want to move in this direction, we can do it incrementally: land this first as a separate API, consider replacing the main isa* functions later.

Ah, I see - sorry, was only looking at the first version of the function that takes To and From. Thanks for helping me see what's going on.

kuhar · 2024-03-06T00:34:50Z

Should this replace the multiple places in SLPVectorizer.cpp that pass classof to all_of/count_if/etc?

@topperc Yes.

Expose function objects that call into `llvm::isa` and `llvm::isa_and_present`, such that these type checks can be used as predicates in generic algorithms. Before this change, `llvm::isa*` functions cannot be easily used without knowing both the argument type and the checked types, which leads to them being wrapped in lambdas. For example: ```c++ llvm::all_of(myTypes, [](auto type) { return llvm::isa<VectorType>(type); }); ``` With this PR the example above becomes: ```c++ llvm::all_of(myTypes, llvm::IsaPred<VectorType>); ``` As an alternative solution, I considered redefining `isa*` as function objects, but I decided against doing that because it would create assymetry with other cast *functions* and could break code that depends on them being actual functions.

For more context on isa predicates, see: llvm/llvm-project#83753. Also clean up some surrounding casts/isas.

For more context on isa predicates, see: llvm#83753.

For more context on isa predicates, see: #83753.

Expose function objects that call into `llvm::isa` and `llvm::isa_and_present`, such that these type checks can be used as predicates in generic algorithms. Before this change, `llvm::isa*` functions cannot be easily used without knowing both the argument type and the checked types, which leads to them being wrapped in lambdas. For example: ```c++ llvm::all_of(myTypes, [](auto type) { return llvm::isa<VectorType>(type); }); ``` With this PR the example above becomes: ```c++ llvm::all_of(myTypes, llvm::IsaPred<VectorType>); ``` As an alternative solution, I considered redefining `isa*` as function objects, but I decided against doing that because it would create asymmetry with other cast *functions* and could break code that depends on them being actual functions.

…es (llvm#87183) For more context on isa predicates, see: llvm#83753.

kuhar requested review from zero9178, dwblaikie, qedawkins, River707 and kazutakahirata March 4, 2024 02:08

llvmbot added the llvm:support label Mar 4, 2024

kuhar mentioned this pull request Mar 4, 2024

[LLVMGPU] Fix allocation space for thread local allocations iree-org/iree#16640

Merged

zero9178 approved these changes Mar 4, 2024

View reviewed changes

dwblaikie approved these changes Mar 6, 2024

View reviewed changes

kuhar added 3 commits March 5, 2024 20:45

Fix typos

9b80de8

Add missing [[nodiscard]] attributes

287da8e

kuhar force-pushed the isa-preds branch from d74118d to 287da8e Compare March 6, 2024 01:46

kuhar merged commit 8fdec5d into llvm:main Mar 6, 2024
3 of 4 checks passed

kuhar added a commit to kuhar/iree that referenced this pull request Mar 31, 2024

[NFC] Simplify filters with isa predicates

74568eb

For more context on isa predicates, see: llvm/llvm-project#83753. Also clean up some surrounding casts/isas.

kuhar mentioned this pull request Mar 31, 2024

[NFC] Simplify type checks with isa predicates iree-org/iree#16935

Merged

kuhar added a commit to iree-org/iree that referenced this pull request Mar 31, 2024

[NFC] Simplify type checks with isa predicates (#16935)

12a0b56

For more context on isa predicates, see: llvm/llvm-project#83753. Also clean up some surrounding casts/isas.

kuhar added a commit to kuhar/llvm-project that referenced this pull request Mar 31, 2024

[SLPVectorizer][NFC] Simplify type checks with isa predicates

bca1f34

For more context on isa predicates, see: llvm#83753.

kuhar mentioned this pull request Mar 31, 2024

[SLP][NFC] Simplify type checks with isa predicates #87182

Merged

kuhar added a commit to kuhar/llvm-project that referenced this pull request Mar 31, 2024

[mlir][NFC] Simplify type checks with isa predicates

383887c

For more context on isa predicates, see: llvm#83753.

kuhar mentioned this pull request Mar 31, 2024

[mlir][NFC] Simplify type checks with isa predicates #87183

Merged

kuhar added a commit that referenced this pull request Mar 31, 2024

[SLP][NFC] Simplify type checks with isa predicates (#87182)

2b0ab05

For more context on isa predicates, see: #83753.

kuhar added a commit that referenced this pull request Apr 1, 2024

[mlir][NFC] Simplify type checks with isa predicates (#87183)

971b852

For more context on isa predicates, see: #83753.

mgehre-amd pushed a commit to Xilinx/llvm-project that referenced this pull request Apr 26, 2024

(Partial backport) [mlir][NFC] Simplify type checks with isa predicat…

79f313a

…es (llvm#87183) For more context on isa predicates, see: llvm#83753.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Support][Casting] Add predicates for `isa*` functions #83753

[Support][Casting] Add predicates for `isa*` functions #83753

kuhar commented Mar 4, 2024

llvmbot commented Mar 4, 2024

zero9178 left a comment

dwblaikie commented Mar 4, 2024

kuhar commented Mar 4, 2024

kuhar commented Mar 5, 2024

dwblaikie commented Mar 5, 2024

kuhar commented Mar 5, 2024 •

edited

dwblaikie commented Mar 5, 2024

kuhar commented Mar 5, 2024

topperc commented Mar 5, 2024

dwblaikie commented Mar 6, 2024

kuhar commented Mar 6, 2024

[Support][Casting] Add predicates for isa* functions #83753

[Support][Casting] Add predicates for isa* functions #83753

Conversation

kuhar commented Mar 4, 2024

llvmbot commented Mar 4, 2024

zero9178 left a comment

Choose a reason for hiding this comment

dwblaikie commented Mar 4, 2024

kuhar commented Mar 4, 2024

kuhar commented Mar 5, 2024

dwblaikie commented Mar 5, 2024

kuhar commented Mar 5, 2024 • edited

dwblaikie commented Mar 5, 2024

kuhar commented Mar 5, 2024

topperc commented Mar 5, 2024

dwblaikie commented Mar 6, 2024

kuhar commented Mar 6, 2024

[Support][Casting] Add predicates for `isa*` functions #83753

[Support][Casting] Add predicates for `isa*` functions #83753

kuhar commented Mar 5, 2024 •

edited