[mlir][llvmir] Added extra builders for CallInstrinsicOp #111664

FMarno · 2024-10-09T11:52:42Z

I've added the extra builders I would like to see for CallIntrinsicOp.
This is inspired by the comment from @antiagainst from here. I am also building Triton.

llvmbot · 2024-10-09T11:53:17Z

@llvm/pr-subscribers-mlir

Author: Finlay (FMarno)

Changes

I've added the extra builders I would like to see for CallIntrinsicOp.
This is inspired by the comment from @antiagainst from here. I am also building Triton.

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

2 Files Affected:

(modified) mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td (+5)
(modified) mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp (+19)

diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
index 88e82ce48959b0..ff5199fe40d124 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
@@ -1943,6 +1943,11 @@ def LLVM_CallIntrinsicOp
     attr-dict
   }];
 
+  let builders = [
+    OpBuilder<(ins "Type": $results, "StringAttr":$intrin, "ValueRange":$args)>,
+    OpBuilder<(ins "TypeRange": $results, "StringAttr":$intrin, "ValueRange":$args, "FastmathFlagsAttr":$fastMathFlags)>
+    ];
+
   let hasVerifier = 1;
 }
 
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index fb7024a14f8d4e..27cbd77f110326 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -3353,6 +3353,25 @@ struct LLVMOpAsmDialectInterface : public OpAsmDialectInterface {
 };
 } // namespace
 
+//===----------------------------------------------------------------------===//
+// CallIntrinsicOp
+//===----------------------------------------------------------------------===//
+
+void CallIntrinsicOp::build(OpBuilder &builder, OperationState &state,
+                            mlir::TypeRange results, mlir::StringAttr intrin,
+                            mlir::ValueRange args,
+                            mlir::LLVM::FastmathFlagsAttr fastMathFlags) {
+
+  build(builder, state, results, intrin, args, fastMathFlags,
+        llvm::ArrayRef<mlir::ValueRange>{});
+}
+void CallIntrinsicOp::build(OpBuilder &builder, OperationState &state,
+                            mlir::Type results, mlir::StringAttr intrin,
+                            mlir::ValueRange args) {
+  build(builder, state, {results}, intrin, args, FastmathFlagsAttr{},
+        llvm::ArrayRef<mlir::ValueRange>{});
+}
+
 //===----------------------------------------------------------------------===//
 // LinkerOptionsOp
 //===----------------------------------------------------------------------===//

llvmbot · 2024-10-09T11:53:18Z

@llvm/pr-subscribers-mlir-llvm

Author: Finlay (FMarno)

Changes

I've added the extra builders I would like to see for CallIntrinsicOp.
This is inspired by the comment from @antiagainst from here. I am also building Triton.

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

2 Files Affected:

(modified) mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td (+5)
(modified) mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp (+19)

diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
index 88e82ce48959b0..ff5199fe40d124 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
@@ -1943,6 +1943,11 @@ def LLVM_CallIntrinsicOp
     attr-dict
   }];
 
+  let builders = [
+    OpBuilder<(ins "Type": $results, "StringAttr":$intrin, "ValueRange":$args)>,
+    OpBuilder<(ins "TypeRange": $results, "StringAttr":$intrin, "ValueRange":$args, "FastmathFlagsAttr":$fastMathFlags)>
+    ];
+
   let hasVerifier = 1;
 }
 
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index fb7024a14f8d4e..27cbd77f110326 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -3353,6 +3353,25 @@ struct LLVMOpAsmDialectInterface : public OpAsmDialectInterface {
 };
 } // namespace
 
+//===----------------------------------------------------------------------===//
+// CallIntrinsicOp
+//===----------------------------------------------------------------------===//
+
+void CallIntrinsicOp::build(OpBuilder &builder, OperationState &state,
+                            mlir::TypeRange results, mlir::StringAttr intrin,
+                            mlir::ValueRange args,
+                            mlir::LLVM::FastmathFlagsAttr fastMathFlags) {
+
+  build(builder, state, results, intrin, args, fastMathFlags,
+        llvm::ArrayRef<mlir::ValueRange>{});
+}
+void CallIntrinsicOp::build(OpBuilder &builder, OperationState &state,
+                            mlir::Type results, mlir::StringAttr intrin,
+                            mlir::ValueRange args) {
+  build(builder, state, {results}, intrin, args, FastmathFlagsAttr{},
+        llvm::ArrayRef<mlir::ValueRange>{});
+}
+
 //===----------------------------------------------------------------------===//
 // LinkerOptionsOp
 //===----------------------------------------------------------------------===//

gysit

Thanks it makes sense adding such builders!

Did you test it downstream? It is a bit hard to see from the code if all the necessary fields of the default constructor are set.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

Lancern

Thanks for improving this!

Lancern · 2024-10-09T12:26:31Z

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

+  let builders = [
+    OpBuilder<(ins "Type": $results, "StringAttr":$intrin, "ValueRange":$args)>,
+    OpBuilder<(ins "TypeRange": $results, "StringAttr":$intrin, "ValueRange":$args, "FastmathFlagsAttr":$fastMathFlags)>
+    ];


Maybe we could also include builders for intrinsics that do not have a result type?

OpBuilder<(ins "StringAttr":$intrin, "ValueRange":$args)> OpBuilder<(ins "StringAttr":$intrin, "ValueRange":$args, "FastmathFlagsAttr":$fastMathFlags)>

Thanks for the review.
I've added those in. Do you know how I could confirm they work? I don't really have a use cause so I was just guessing.

Another approach might be adding a unit test under mlir/unittests/Dialect/LLVMIR. But it seems like few people ever do this ... Anyway this builder is fairly simple and I believe it should be OK if no reviewers have problems about it.

FMarno · 2024-10-09T16:15:43Z

@gysit Thank you for the review, I've addressed all your comments.

Did you test it downstream? It is a bit hard to see from the code if all the necessary fields of the default constructor are set.

Yes, it works for code gen in Triton. If there is a better way to test builders then I'd love to know.

gysit

LGTM modulo nits!

The only easy way to test builders is using them in a pass/canonicalization AFAIK. Since there are not many uses of this op upstream that does probably not work here.

gysit · 2024-10-09T16:35:03Z

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

@@ -3324,6 +3324,34 @@ LogicalResult CallIntrinsicOp::verify() {
  return success();
 }

+void CallIntrinsicOp::build(OpBuilder &builder, OperationState &state,
+                            mlir::StringAttr intrin, mlir::ValueRange args) {
+  build(builder, state, /*results=*/Type{}, intrin, args, FastmathFlagsAttr{},


Suggested change

build(builder, state, /*results=*/Type{}, intrin, args, FastmathFlagsAttr{},

build(builder, state, /*results=*/TypeRange{}, intrin, args, FastmathFlagsAttr{},

I think this is a type range (probably Type converts to TypeRange)?

Thanks, I've addressed that.

gysit · 2024-10-09T16:35:23Z

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

+void CallIntrinsicOp::build(OpBuilder &builder, OperationState &state,
+                            mlir::StringAttr intrin, mlir::ValueRange args,
+                            mlir::LLVM::FastmathFlagsAttr fastMathFlags) {
+  build(builder, state, /*results=*/Type{}, intrin, args, fastMathFlags,


Suggested change

build(builder, state, /*results=*/Type{}, intrin, args, fastMathFlags,

build(builder, state, /*results=*/TypeRange{}, intrin, args, fastMathFlags,

@antiagainst

Extra builders for CallIntrinsicOp. This is inspired by the comment from @antiagainst from [here](llvm#108933 (comment)).

[mlir][llvmir] Added extra builders for CallInstrinsicOp

6be01c8

FMarno requested review from antiagainst, gysit, victor-eds and Lancern October 9, 2024 11:52

llvmbot added mlir:llvm mlir labels Oct 9, 2024

gysit reviewed Oct 9, 2024

View reviewed changes

Lancern reviewed Oct 9, 2024

View reviewed changes

FMarno added 2 commits October 9, 2024 17:05

fixup! [mlir][llvmir] Added extra builders for CallInstrinsicOp

a5d76f1

fixup! [mlir][llvmir] Added extra builders for CallInstrinsicOp

f9c3cf4

Added a couple more builders for call instrinsic op

01987e9

FMarno requested review from gysit and Lancern October 9, 2024 16:24

gysit approved these changes Oct 9, 2024

View reviewed changes

fixup! Added a couple more builders for call instrinsic op

1e7bd93

Lancern approved these changes Oct 10, 2024

View reviewed changes

FMarno merged commit 741ad3a into llvm:main Oct 10, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mlir][llvmir] Added extra builders for CallInstrinsicOp #111664

[mlir][llvmir] Added extra builders for CallInstrinsicOp #111664

FMarno commented Oct 9, 2024

llvmbot commented Oct 9, 2024

llvmbot commented Oct 9, 2024

gysit left a comment

Lancern left a comment

Lancern Oct 9, 2024

FMarno Oct 9, 2024

Lancern Oct 10, 2024

FMarno commented Oct 9, 2024 •

edited

Loading

gysit left a comment

gysit Oct 9, 2024

FMarno Oct 9, 2024

gysit Oct 9, 2024

	build(builder, state, /results=/Type{}, intrin, args, FastmathFlagsAttr{},
	build(builder, state, /results=/TypeRange{}, intrin, args, FastmathFlagsAttr{},

[mlir][llvmir] Added extra builders for CallInstrinsicOp #111664

[mlir][llvmir] Added extra builders for CallInstrinsicOp #111664

Conversation

FMarno commented Oct 9, 2024

llvmbot commented Oct 9, 2024

llvmbot commented Oct 9, 2024

gysit left a comment

Choose a reason for hiding this comment

Lancern left a comment

Choose a reason for hiding this comment

Lancern Oct 9, 2024

Choose a reason for hiding this comment

FMarno Oct 9, 2024

Choose a reason for hiding this comment

Lancern Oct 10, 2024

Choose a reason for hiding this comment

FMarno commented Oct 9, 2024 • edited Loading

gysit left a comment

Choose a reason for hiding this comment

gysit Oct 9, 2024

Choose a reason for hiding this comment

FMarno Oct 9, 2024

Choose a reason for hiding this comment

gysit Oct 9, 2024

Choose a reason for hiding this comment

FMarno commented Oct 9, 2024 •

edited

Loading