[llvm] [Support][Casting] Add predicates for `isa*` functions (PR #83753)

Jakub Kuderski via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 5 13:23:37 PST 2024


kuhar wrote:

> 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:
```console
➜ 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) &&
```
```console
➜  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.

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


More information about the llvm-commits mailing list