-
Notifications
You must be signed in to change notification settings - Fork 84
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
Add operation builders #26
Conversation
Thank you Mathieu. Does this PR have test cases that demonstrate the use of this. In another project, I am currently trying to build operations (https://github.com/compiling-techniques/ChocoPyCompiler/pull/76) and the construction of such operations is very verbose. Does this patch help to reduce this verbosity, in particular the need to call get_ssa_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.
LGTM. I am really looking forward to using this as well :)
def get(operand1: Union[Operation, SSAValue], | ||
operand2: Union[Operation, SSAValue]) -> Addi: | ||
return Addi.build(operands=[operand1, operand2], | ||
result_types=[IntegerType.from_width(32)]) |
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.
Do MLIR addis only work on 32bit integers? Maybe add a size argument that has default.
Also, i jus t looked at the MLIR docu, is it possible that simple arithmetic got taken out of std
into arith
? Maybe we should do the same (or rename them to arith.
instead of std.
).
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.
The type should be a parameter as will, I think.
MLIR is currently refactoring this, so yes, we should probably introduce a arith
dialect at some point.
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.
Maybe we can make the result_type
an optional parameter. 32 bit will be the one type we use most frequently for now, but later on it would be nice to have this flexibility.
On the other hand, one has still access to the Addi.build
factory.
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.
Yeah, I kept the operation as it was before, but I think we should overall make an additional pass later on the dialects. For example, affine is not up to date at all, and here std is a bit broken.
regions=[]) -> T: | ||
"""Create a new operation using builders.""" | ||
... | ||
|
||
def add_region(self, region: Region) -> None: | ||
self.regions.append(region) | ||
region.parent = self |
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.
(I guess I cannot comment on lines that are too far away from the changed ones)
Maybe add a build to block aswell? from_op_list or from_op_list_and_args.
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.
This should be done now!
Yes, thin gets transformed to
and the constructors
gets replaced by
|
I added tests for operation builders. |
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.
This looks good to me.
I think you can merge this and release 0.1. The changes for ChocoPy can be implemented later on, the requirements.txt will still specify the previous version.
operands=[[], []], | ||
result_types=[MemRefType.from_type_and_list(return_type, shape)], | ||
attributes={ | ||
"alignment": IntegerAttr.from_int_and_width(alignment, 64) |
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.
I honestly do not remember why I selected 64 as a default value here. Is this indeed a good choice?
Maybe it would make sense to integrate #29 into the release of 0.1 as well. It will introduce a lot of breaking changes. |
I removed the default bitwidth for the IntegerAttr attribute. I think I'm done with this branch otherwise, and if this change is okay we can merge it. |
Oh, it also seems that yapf just got updated, and that now it add a line skip after a class definition? I'll see if I can disable this for now |
Or we could also all upgrade to the last yapf version? |
Upgrading yapf and then merging this branch seems reasonable. |
Oh sorry I modified this too fast, I'll remove this commit |
Changing it is also not a problem for me. Do whatever you prefer. |
Operation
build
act ascreate
but call the relevant builders for attributes and results.This improve the number of arguments we can give to our builders, and reduce a bit the boilerplate.
The constructors for operations are also moved to the operation classes, which makes more sense.