-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[mlir][LLVM] Add disjoint flag #115855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mlir][LLVM] Add disjoint flag #115855
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -93,6 +93,26 @@ class LLVM_IntArithmeticOpWithExactFlag<string mnemonic, string instName, | |||||||||||||||||||||
| "$res = builder.Create" # instName # | ||||||||||||||||||||||
| "($lhs, $rhs, /*Name=*/\"\", op.getIsExact());"; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| class LLVM_IntArithmeticOpWithDisjointFlag<string mnemonic, string instName, | ||||||||||||||||||||||
| list<Trait> traits = []> : | ||||||||||||||||||||||
| LLVM_ArithmeticOpBase<AnySignlessInteger, mnemonic, instName, | ||||||||||||||||||||||
| !listconcat([DeclareOpInterfaceMethods<DisjointFlagInterface>], traits)> { | ||||||||||||||||||||||
| let arguments = !con(commonArgs, (ins UnitAttr:$isDisjoint)); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| string mlirBuilder = [{ | ||||||||||||||||||||||
| auto op = $_builder.create<$_qualCppClassName>($_location, $lhs, $rhs); | ||||||||||||||||||||||
| moduleImport.setDisjointFlag(inst, op); | ||||||||||||||||||||||
| $res = op; | ||||||||||||||||||||||
| }]; | ||||||||||||||||||||||
| let assemblyFormat = [{ | ||||||||||||||||||||||
| (`disjoint` $isDisjoint^)? $lhs `,` $rhs attr-dict `:` type($res) | ||||||||||||||||||||||
| }]; | ||||||||||||||||||||||
| string llvmBuilder = | ||||||||||||||||||||||
| [{auto inst = builder.Create}] # instName # | ||||||||||||||||||||||
| [{($lhs, $rhs, /*Name=*/""); | ||||||||||||||||||||||
| moduleTranslation.setDisjointFlag(op, inst); | ||||||||||||||||||||||
| $res = inst;}]; | ||||||||||||||||||||||
|
||||||||||||||||||||||
| string llvmBuilder = | |
| [{auto inst = builder.Create}] # instName # | |
| [{($lhs, $rhs, /*Name=*/""); | |
| moduleTranslation.setDisjointFlag(op, inst); | |
| $res = inst;}]; | |
| string llvmBuilder = [{ | |
| auto inst = builder.Create}] # instName # [{($lhs, $rhs, /*Name=*/""); | |
| moduleTranslation.setDisjointFlag(op, inst); | |
| $res = inst; | |
| }]; |
nit: an attempt to format tablegen a bit nicer, feel free to pick it if it works or ignore it otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to work, done.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -689,6 +689,15 @@ void ModuleImport::setExactFlag(llvm::Instruction *inst, Operation *op) const { | |||||||||||||||||||
| iface.setIsExact(inst->isExact()); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| void ModuleImport::setDisjointFlag(llvm::Instruction *inst, | ||||||||||||||||||||
| Operation *op) const { | ||||||||||||||||||||
| auto iface = cast<DisjointFlagInterface>(op); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| auto inst_disjoint = cast<llvm::PossiblyDisjointInst>(inst); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| iface.setIsDisjoint(inst_disjoint->isDisjoint()); | ||||||||||||||||||||
|
||||||||||||||||||||
| auto iface = cast<DisjointFlagInterface>(op); | |
| auto inst_disjoint = cast<llvm::PossiblyDisjointInst>(inst); | |
| iface.setIsDisjoint(inst_disjoint->isDisjoint()); | |
| auto iface = cast<DisjointFlagInterface>(op); | |
| auto inst_disjoint = cast<llvm::PossiblyDisjointInst>(inst); | |
| iface.setIsDisjoint(inst_disjoint->isDisjoint()); |
nit: I think it is fine to drop the newlines here. At the very least the one between the two cast instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1898,6 +1898,14 @@ void ModuleTranslation::setLoopMetadata(Operation *op, | |||||||||||||||||||
| inst->setMetadata(llvm::LLVMContext::MD_loop, loopMD); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| void ModuleTranslation::setDisjointFlag(Operation *op, llvm::Value *inst) { | ||||||||||||||||||||
|
||||||||||||||||||||
| void ModuleTranslation::setDisjointFlag(Operation *op, llvm::Value *inst) { | |
| void ModuleTranslation::setDisjointFlag(Operation *op, llvm::Instruction *inst) { |
just saw this. Can this be llvm::Instruction as well. Otherwise we should probably rename inst to value or so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I try using an instruction here, I get error: invalid conversion from ‘llvm::Value*’ to ‘llvm::Instruction*’ so I`ll rename inst to value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| auto inst_disjoint = cast<llvm::PossiblyDisjointInst>(inst); | |
| auto disjointInst = cast<llvm::PossiblyDisjointInst>(inst); |
nit: missed this before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename inst_disjoint in the other function as well? Sorry I forgot to mark that as well. Feel free to fix this things proactively.
Regarding the llvm::Value. I think you could use llvm::Instruction and it would cast to a llvm::Value right? That maybe preferable over using a value since it is in line with all the other helper functions we have. If it doesn't work the comment on the function needs to be fixed by replacing inst -> value.
Sorry for being a bit pedantic :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forgot about the other function.
As for the Value issue, I may have been unclear, but the error I get if I use Instruction is from the llvm builder
/home/leon_frenot/fork-llvm/build/tools/mlir/include/mlir/Dialect/LLVMIR/LLVMConversions.inc:279:43: error: invalid conversion from ‘llvm::Value*’ to ‘llvm::Instruction*’ [-fpermissive]
279 | moduleTranslation.setDisjointFlag(op, inst);
| ^~~~
| |
| llvm::Value*
So the issue comes from builder.CreateOr returning a Value instead of an Instruction.
For now I'll simply fix the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense!
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| auto iface = cast<DisjointFlagInterface>(op); | |
| auto inst_disjoint = cast<llvm::PossiblyDisjointInst>(inst); | |
| inst_disjoint->setIsDisjoint(iface.getIsDisjoint()); | |
| auto iface = cast<DisjointFlagInterface>(op); | |
| auto inst_disjoint = cast<llvm::PossiblyDisjointInst>(inst); | |
| inst_disjoint->setIsDisjoint(iface.getIsDisjoint()); |
nit: here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| ; RUN: mlir-translate -import-llvm -split-input-file %s | FileCheck %s | ||
|
|
||
| ; CHECK-LABEL: @disjointflag_inst | ||
| define void @disjointflag_inst(i64 %arg1, i64 %arg2) { | ||
| ; CHECK: llvm.or disjoint %{{.*}}, %{{.*}} : i64 | ||
| %1 = or disjoint i64 %arg1, %arg2 | ||
| ret void | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| // RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s | ||
|
|
||
| // CHECK-LABEL: define void @disjointflag_func | ||
| llvm.func @disjointflag_func(%arg0: i64, %arg1: i64) { | ||
| // CHECK: %{{.*}} = or disjoint i64 %{{.*}}, %{{.*}} | ||
| %0 = llvm.or disjoint %arg0, %arg1 : i64 | ||
| llvm.return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.