[CIR] [CodeGen] Introduce IsFPClassOp to support builtin_isfpclass

[ChuanqiXu9]

The llvm's intrinsic llvm.is.fpclass is used to support multiple float point builtins: https://clang.llvm.org/docs/LanguageExtensions.html#builtin-isfpclass

The __builtin_isfpclass() builtin is a generalization of functions
isnan, isinf, isfinite and some others defined by the C standard. It tests
if the floating-point value, specified by the first argument, falls into
any of data classes, specified by the second argument.

I meant to support this by creating IntrinsicCallOp directly. But I can't make it due to #480 since the return type of the intrinsic will mismatch. So I have to create a new Op for it. But I feel it might not be too bad. At least it is more explicit and more expressive.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[Lancern]

One last nit and the rest of this PR LGTM!

[ghehg]

The llvm's intrinsic llvm.is.fpclass is used to support multiple float point builtins: https://clang.llvm.org/docs/LanguageExtensions.html#builtin-isfpclass

The __builtin_isfpclass() builtin is a generalization of functions
isnan, isinf, isfinite and some others defined by the C standard. It tests
if the floating-point value, specified by the first argument, falls into
any of data classes, specified by the second argument.

I meant to support this by creating IntrinsicCallOp directly. But I can't make it due to #480 since the return type of the intrinsic will mismatch. So I have to create a new Op for it. But I feel it might not be too bad. At least it is more explicit and more expressive.

Thank you for looking into the issue! Really glad you are offering a hand to tackle builtins and intrinsics.
However, I believe it is still better to just use IntrinsicCallOp, not introducing new CIR Op.
The thing is, if the only problem is intrinsic function signature mismatch, we could always work around at LLVM Lowering, instead of introducing a new type of OP to solve the problem.
For example, we could call getI1Type during lowering to give i1 result type and choose mlir::LLVM::IsFPClass during lowering.
You might feel uncomfortable giving special ad-hoc treatment in LLVM lowering, but creating a CIR OP just for a specific intrinsic (there are many such intrinsic ops in LLVM dialect, I doubt if we want have 1-1 match of them in CIR) is much more of special ad-hoc treatment.
That being said, changes to CIRTypes.td is very good and needed by itself!

[ghehg]

I'd like us try to use IntrinsicCallOp instead of introducing new CIR Op.
details below.
#971 (comment)

[ChuanqiXu9]

You might feel uncomfortable giving special ad-hoc treatment in LLVM lowering, but creating a CIR OP just for a specific intrinsic (there are many such intrinsic ops in LLVM dialect, I doubt if we want have 1-1 match of them in CIR) is much more of special ad-hoc treatment.

@ghehg in mlir, we have https://github.com/llvm/llvm-project/blob/bb89988174e5f1a9a419637cadae07e4e8c61c3e/mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td#L103-L107

So I think it makes sense to match their structure there and I think a new CIR Op maybe helpful to improve the readability and more expressive.

[bcardosolopes]

@ghehg thanks for the review. I think I see where you come from for the general point of view, but this sounds like a nice abstraction (and it already covers a bunch of other builtins). The level of abstraction that @ChuanqiXu9 picked feels good to me. I'm also a bit confused with your suggestion for using IntrinsicCallOp given that LLVM even has a specialized instruction to lower this: mlir::LLVM::IsFPClass, given that IntrinsicCallOp is usually a handy for 1-1 to actual LLVM intrinsic calls (not the case here).

[ghehg]

@ghehg thanks for the review. I think I see where you come from for the general point of view, but this sounds like a nice abstraction (and it already covers a bunch of other builtins). The level of abstraction that @ChuanqiXu9 picked feels good to me. I'm also a bit confused with your suggestion for using IntrinsicCallOp given that LLVM even has a specialized instruction to lower this: mlir::LLVM::IsFPClass, given that IntrinsicCallOp is usually a handy for 1-1 to actual LLVM intrinsic calls (not the case here).

Oh, sorry for IntrinsicCallOp confusion ( I probably should change the name of CIR::IntrinsicCallOp to CIR::LLVMIntrinsicOp or something). What I meant is that I imagine that CIR::IntrinsicCallOp should not be a 1-1 match to LLVM::CallIntrinsicOp, instead, it should represent all intrinsic calls that we will leave to LLVM, and lowering could choose to use LLVM::CallIntrinsicOp or other LLVM dialect llvm.intri ops Operations for LLVM IR Intrinsics.
Otherwise, there are dozens of llvm.intr LLVM Ops, and I don't think we want to them all have corresponding CIR Ops, that would defeat the purpose of having CIR as higher level IR.
Let me know what you think,

[ghehg]

You might feel uncomfortable giving special ad-hoc treatment in LLVM lowering, but creating a CIR OP just for a specific intrinsic (there are many such intrinsic ops in LLVM dialect, I doubt if we want have 1-1 match of them in CIR) is much more of special ad-hoc treatment.

@ghehg in mlir, we have https://github.com/llvm/llvm-project/blob/bb89988174e5f1a9a419637cadae07e4e8c61c3e/mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td#L103-L107

So I think it makes sense to match their structure there and I think a new CIR Op maybe helpful to improve the readability and more expressive.

That's exactly my concern that we just move LLVM Ops into CIR, especially given LLVM dialect has dozens of such intrinsic Ops.
We'd like to keep CIR as a higher level IR. However, if you feel a need to introduce this Op for goo, I'll let it go.

sounds like we think this Op has enough abstraction level to be added into CIR, so let's have it then!

[bcardosolopes]

What I meant is that I imagine that CIR::IntrinsicCallOp should not be a 1-1 match to LLVM::CallIntrinsicOp

@ghehg The sole purpose of IntrinsicCallOp is to forward things 1-1 to LLVM intrinsics, mainly because we have tons of LLVM intrinsics that wouldn't have any specific higher level meaning and are not worth getting their specific op.

[bcardosolopes]

LGTM after minor style fix

[bcardosolopes]

Good to go once conflict is fixed

[bcardosolopes]

Oh boy, I was reviewing/merging in chronological order and you just got bitten by refactoring again!

[ChuanqiXu9]

I saw the main was force updated. So I have to rebase any way : ) Hope we can work on the real trunk some day : )

[bcardosolopes]

Hope we can work on the real trunk some day

That's going to be speed up pretty soon, stay tuned!