xdslproject / xdsl Goto Github PK

These types tend to get long. For readability, it is useful to bring the interesting things to the front.

Currently Attributes and Operations use either verify or verify_ as according method names. This can lead to confusion when one tries to implement a custom verifier.

Currently, the rewrite walker creates new blocks for each existing block but does not introduce BlockArguments. Furthermore, this should be added to the bookkeeping part.

I cannot assign this to anyone, but I'll look into that as I require this feature.

I'm not sure if we really want terminators in xdsl, but they might be necessary for stronger verification passes.
We do not enforce terminators in ChocoPy so far, which leads to issues while lowering.

Just a small list of minor upgrades that can help improve the structure of the xdsl repo.
I am not assigning this to anyone, just listing things that can help a lot in automating minor stuff for long term.
Happy to discuss.

https://github.com/orgs/xdslproject/projects/2

The following breaks with a Exception 1 parameters expected, got 0:

@irdl_attr_definition
class Foo(ParametrizedAttribute):
  name: str = "bar.foo"
  param = ParameterDef(AnyAttr())

# Try to construct
Foo([*SomeAttr*])

Removing the type annotation for the name field fixes it:

@irdl_attr_definition
class Foo(ParametrizedAttribute):
  name = "bar.foo"
  param = ParameterDef(AnyAttr())

# Try to construct
Foo([*SomeAttr*])

I think we should change the API s.t. names are not defined as fields. That would make it less error-prone. Maybe have the name of the Attribute as part of the decorator? irdl_attr_definition("bar.foo")

It seems that MLIR changed the way they store information in the *_segment_sizes attributes. In the case of cond_br they expect an entry for all the operand "kinds", even for non-variadic ones.

For some operations/types it would be nice to have optional attributes or unit attributes/parameters.

For example, MLIR's memref.global has a unit attribute constant which, when present, indicates that this global is constant.
Another one are IntegerTypes, which can have a "signedness" parameter which indicates that the type is a signed integer.

One could also add boolean attributes but this might clutter the output a lot.

The affine dialect still uses new_op instead of actual classes. Furthermore, it is nowhere near feature completion as IntegerSets are not yet supported.
Either we change the existing operations to use IRDL or we drop the dialect completely. What do you think @math-fehr ?

The default parser expects type annotations for the arguments of an operand. This seems unnecessarily verbose:

Instead of the code:

  %0 : !riscv_ssa.reg = riscv_ssa.li() ["immediate" = 42 : !i64]
  %1 : !riscv_ssa.reg = riscv_ssa.mul(%0 : !riscv_ssa.reg , %0 : !riscv_ssa.reg)

I would prefer to write

  %0 : !riscv_ssa.reg = riscv_ssa.li() ["immediate" = 42 : !i64]
  %1 : !riscv_ssa.reg = riscv_ssa.mul(%0, %0)

The MLIRConverter currently struggles with the CF dialect on some things related to operand_segment_sizes.

Currently, IntAttr is defined in builtin but there is also a IntegerAttr. I think we should either remove IntAttr or rename it to something else.
This caused quite some confusion, e.g., https://github.com/compiling-techniques/ChocoPyCompiler/pull/75/files

Right now, after deletion, operations can still be accessed.
It would be nice to raise exceptions when deleted operations are accessed.

Printing and requiring types after an operand use makes the IR unnecessarily verbose and hard to read. MLIR does not do this either. Hence, I wonder, should we maybe just not do this?

Currently, the parsing and printing is slightly broken.
First, raised by #3, we cannot print an operation/module to a string, but allowing to simply write print(op) would be nice.

However, this raises multiple issues:

• How to print an operation that refers to an external SSA value? Should we just give them a name like as UNK0, to show explicitely that the value is outside?
• Then, how to print a list of operation? If the second operation use a result of the first, we don't want to print an UNK for instance. I believe that in that case, we might have to use the Printer explicitly in some way.

SSAValue.erase() raises a generic exception "Attempting to delete SSA value that still has uses."

It would be more useful if it also printed out the specific operation.

This can be achieved by:

   def erase(self, safe_erase: bool = True) -> None:
        """
        Erase the value.
        If safe_erase is True, then check that no operations use the value anymore.
        If safe_erase is False, then replace its uses by an ErasedSSAValue.
        """
        if safe_erase and len(self.uses) != 0:
            raise Exception(
                f"Attempting to delete SSA value that still has uses: {self}")
        self.replace_by(ErasedSSAValue(self.typ))

Implement named SSAValues (i.e. SSAValues get an optional name, which they keep when being printed)

MLIR changed parts of their dialect structures by introducing an arith dialect. We should reflect this new dialect as well to ensure we stay compatible with MLIR.

currently, we just do parent.parent.parent kinds of things. Add parent_op, parent_region, and parent_block.

Currently, we just return an exception when there is an error in the input program, or when a verifier fails.
It would be nice to have a way to print back the program, with the error pointing to the right place.

If one currently forgets to add a referenced operation to a region, i.e., if it isn't present in the output, the printer fails with a very nasty error message.

A small check around https://github.com/xdslproject/xdsl/blob/main/src/xdsl/printer.py#L76-L79 might already be sufficient.

I would prefer to parse the type after an assignment to be more in line with what MLIR uses. I also find the delayed type easier to read as the interesting information, the operation that we call, is frontloaded.

Instead of:

%0 : !riscv_ssa.reg = riscv_ssa.li() ["immediate" = 42 : !i64]
%1 : !riscv_ssa.reg = riscv_ssa.mul(%0 : !riscv_ssa.reg, %0 : !riscv_ssa.reg)

I would expect to read:

%0 = riscv_ssa.li() ["immediate" = 42 : !i64] : !riscv_ssa.reg 
%1 = riscv_ssa.mul(%0 : !riscv_ssa.reg, %0 : !riscv_ssa.reg) : !riscv_ssa.reg 

On my thesis project I have many prints of a module as it is rewritten (see below), where the module is output with printer.print_op(module).

Following a meeting with Tobais and Mathieu, we think it would be incredible helpful if the printer could implement functionality for highlighting and arrows. Highlighting could simply be done by changing the output colour, and arrows could use a string such as "->".

If anyone has any input on this, or any additional printer features they would like added, I would love to hear!

Also, sorry if I have posted this in the wrong place.

Currently when the printer encounters an SSAValue that is not attached to the IR, e.g. a missing operand, it throws an exception and fails. For debugging purposes it would be very helpful if it could still print the whole IR with a string similar to MISSING_SSA_VALUE instead of the missing value where it fails and throw the Exception afterwards. This can be done similarly in MLIR and, seeing the whole context, helps with debugging.

This is the part I am refering to

It seems that code outside of the xdsl project cannot use op_type_rewrite_pattern. The decorator triggers an error.
From what I saw, the reason is that the decorator does not get the hint as a class, but a string.

Currently, if we have two regions, we can't reuse names between regions.
For instance:

func.func () {
  %a = ...
}

func.func () {
  %a = ... // This will trigger an error : `SSAValue "a" is already defined`
}

I believe this would also be a problem for Basic block names

Moving a lot of the choco-opt boilerplate code into xDSL might help to construct other opt like tools for filecheck tests.

The parent attribute does not get set for a lot of operations. This seems to happen because Blocks are constructed by hand, which does not set the parent attribute. AFAIU As the region factory function internally uses the block factory, even constructing regions does not set the parent attribute.

Currently, the walker simply duplicates the BlockArguments of Blocks it traverses, independent of any changes a pattern might have caused. When a pattern modifies the arguments the pattern does not notice and thus does not bookkeep them.

I guess one could somehow integrate this into the RewriteAction, but it might be costly to detect these changes. On the other hand, informing the RewriteAction manually about every change might be cumbersome and error-prone.

Note: This might be solvable by introducing some kind of Rewriter, as MLIR has it, which either replaces or changes operations. The MLIR rewriter must be used to perform any modification on the IR, which in turns allows to introduce arbitrary hooks from the framework side, as all actions will go through it.

We currently do not support comments when parsing xdsl. Our parser should gain support to ignore comments.

Many of the pytests are quite old and were not updated cleanly. Some of them can be migrated to filecheck tests, while others do not require such complicated handling of dialect instances.

As discussed in #60, we might want to emit an error while verifying in this case, not just when printing.

It would be very cool to have easy support for defining custom parser and printer. In particular, some of the tree-based code becomes very ugly, there defining custom syntax like the following would be very helpful:

op_with_regions()
with first_region = {
  *region content*
}
with second_region = {
  *region content*
}

I might just be out of the loop on these things, is there some support for this already, @math-fehr ?

We would like to write something like:

@irdl_attr_definition
class ChocoListTypeData:
    name = "choco.list_type_data"
    # The following is a reference to itself
    type = AttributeDef(AnyOf([ChocoTypeData, ChocoListTypeData]))

Some of the jupyter notebooks currently have parts in them, that trigger huge error prints, since the build API checks for the number of results and so on. Maybe update these such that the error prints are nicer.

ChocoPy has a visitor which is quite useful to work through the IR, and a copy of this is at https://github.com/xdslproject/psy/blob/main/util/visitor.py . Whilst the chocoPy visitor imported the choco dialects this isn't actually needed and the visitor is dialect independent.

Therefore it might be a good idea to bring into xDSL proper as it seems like generic functionality. I guess there are two questions related to this:

1. We can do visiting of the dialect with PatternRewriteWalker already, so I guess that sort of does the job of the visitor but from the naming it seems like the PattenRewriter should be doing some rewriting of the IR, whereas the visitor can be collecting information only. Therefore the visitor is lighter-weight?

2. Currently the visitor matches methods based on their pot_hole case name vs the dialect's CamelCase class name. This works fine, but isn't ideal I think from a generality POV and we might want to modify to match on the type of the method's IR node argument instead (like the PatternRewriteWalker does).

Currently the mlir_converter.py is a mess. When we want to use it from the ChocoPy project, we have to rewrite parts of it to make it possible to wrap it into a tool.

Devito uses a multitude of tree visitors that only triggers matches on specific tree structures.
For example, they do only match on Iteration nodes that do not contain any children nodes of type Iteration.

Supporting such more complex patterns out of the box might make many Devito transforms easier to implement.

Currently, the lit tool marks a test as failed as soon as one of the commands in the pipeline terminates with non-zero exit code. This makes it impossible to check for certain diagnostics/exceptions.

This could be solved by improving diagnostics support and adding a kind of verify-diagnostics flag (as mlir-opt does) to then print diagnostics or exception traces while still exiting with zero. I'm not yet sure how this can be integrated nicely into the opt tool though.

Note: there is already a hack in choco-opt to capture exceptions to then print the message. This is not clean in my opinion. Maybe we could define certain exception types that will be ignored with a certain flag.

It may be useful to use xDSL for out-of-tree MLIR dialects in user-defined rewritings. One might think that one can simply import mlir_mydialect in addition to import mlir but this is fundamentally impossible.

The problem is that out-of-tree MLIR dialects are compiled as native (C++) libraries, which are exposed to Python as native modules (e.g., mlir_mydialect) that are incompatible with the in-tree MLIR module (mlir). The underlying reason is that some objects in MLIR are per-library singletons, so different libraries each have their own singleton instances. The recommended way to compile out-of-tree Python modules is therefor to include all of MLIR core such that all MLIR-related dependencies are included in the out-of-tree Python module. Users of such a library then have to import mlir_mydialect instead of import mlir everywhere in their code.

In xDSL, this means that the main xDSL Python package (i.e., the one from this repository) needs to import a different Python package for mlir depending on what dialects the user of the package want to use . In other words, xdsl needs to import <desire-mlir-package> based on what the code that does import xdsl desires. The import keyword, however, does not allow to import packages dynamically (i.e., based on the value of a variable), so there is not trivial way to achieve the desired goal.

This issue is an attempt to express the problem clearly and provide a place to find the best solution to the problem. #109 and #110 are two initial attempts.

Currently, we have a VectorAttr, but MLIR calls them DenseIntOrFPElementsAttr and parametrizes them with either a VectorType or a TensorType. As we require this flexibility for the MLIR conversion, we have to reflect this structure in our implementation as well.

It would be nice if the printer could print to a string.

Currently builtin's IntegerType has no signedness information, thus all integers are treated as unsigned integers, which is a limitation for our lowering to mlir.