In D76903#1954105, @mehdi_amini wrote:

In D76903#1954082, @bondhugula wrote:

In D76903#1948918, @mehdi_amini wrote:

(I don't even know why we allow such operation name in the first place to be honest, other than saving a few chars when writing tests there isn't much interest)

One advantage has been in writing test cases where you need an op with a side-effect so that it just doesn't become dead code (when canonicalization is being run as part of the pass/utility you are testing). These unknown ops provided the easiest/shortest IR for it - we would have otherwise had to think of another side-effecting op (like creating a memref type and a load/store on it). Any other quick shortcut replacements?

Using a name with a dot?
foo.myop instead of ˋmyop` should give the same effect I expect.

In D76903#1948918, @mehdi_amini wrote:

(I don't even know why we allow such operation name in the first place to be honest, other than saving a few chars when writing tests there isn't much interest)

In D76602#1951483, @nicolasvasilache wrote:

@bondhugula
Thanks for the refactoring!

Dropping the LLVM flag and refactoring the impl further sounds like the right thing to do IMO.
The unit test that uses the flag can easily be updated (or deprecated in favor of your test).
The internal use case we have for this will be easy to update.

clang format

Address review comment

Ping reviewers - @jdoerfert - thanks!

Thanks for fixing this. In fact, I had review comments along the lines you address on an earlier patch that introduced these but looks like they didn't get in. (For consistency, we do need the ability to create 0 result maps that have an arbitrary number of dimensions and symbols, which your patch addresses.)

Thanks for the review.

Update op documentation.

In D71961#1948401, @mehdi_amini wrote:

FYI I still believe that a good first step her would be to use something like the FunctionLike trait to constraint the parent of the return op.

In D76637#1951713, @mravishankar wrote:

@bondhugula : Gentle ping on this patch. I think it is ready to land now. PTAL.

In D76975#1951247, @xbolva00 wrote:

Prefix patch with [GVN] please.

OK.

changes after rebase - unregistered ops

Rebase.

Ping reviewers @herhut @ftynse - thanks.

fix eraseBlock iteration order

Address review comments.

More test cases.

Address review comment - add comment.

add attribute only if the alignment is a power of two

Thanks for the review, @jdoerfert Done.

Address review comments; add alignment attribute to aligned_alloc sites

Thanks very much for fixing all these!

This has been split into five patches ending with D76976.

In D76903#1948937, @rriddle wrote:

In D76903#1948918, @mehdi_amini wrote:

In D76903#1948894, @bondhugula wrote:

In D76903#1948876, @mehdi_amini wrote:

In D76903#1948842, @bondhugula wrote:

The commit summary opening "By default, the verifier won't allow operations with unregistered dialect. " reads as if this was the situation before.

I used the future tense here to indicate the change.

Besides "operations with unregistered dialect", what about unregistered operations, i.e., operations that aren't part of any registered dialect?

To me “operations with unregistered dialect” is the same as “ operations that aren't part of any registered dialect”.

But the two are clearly different. There could be an operation that is part of a dialect that is not registered, and you then could have operations that aren't part of any dialect like "foo"() : () -> () -- the latter isn't part of say any dialect.

OK I see what you mean, I'll write “operations that aren't part of any registered dialect” which covers both f.oo which if part of the unregistered dialect f and foo which is not part of any dialect. The verifier is covering both here.

(I don't even know why we allow such operation name in the first place to be honest, other than saving a few chars when writing tests there isn't much interest)

This is wrong. Operations are always part of a dialect. "foo" would be considered to be part of the builtin dialect, which currently has no namespace. IMO we should set the namespace of the builtin dialect to "mlir" and the assert that operation names
always have a dialect prefix.

In D76903#1948876, @mehdi_amini wrote:

In D76903#1948842, @bondhugula wrote:

The commit summary opening "By default, the verifier won't allow operations with unregistered dialect. " reads as if this was the situation before.

I used the future tense here to indicate the change.

Besides "operations with unregistered dialect", what about unregistered operations, i.e., operations that aren't part of any registered dialect?

To me “operations with unregistered dialect” is the same as “ operations that aren't part of any registered dialect”.

The commit summary opening "By default, the verifier won't allow operations with unregistered dialect. " reads as if this was the situation before. Consider rephrasing please. "Change the verifier to disallow ..."? Besides "operations with unregistered dialect", what about unregistered operations, i.e., operations that aren't part of any registered dialect? It looks like you are equating the two, and did you mean to use the latter for this patch?

In D76976#1948652, @fhahn wrote:

Please prefix the commit message with something like [NewGVN]

perfect nest check

Take out any yield op changes. Rebase.

Rebase

Update child revision

Thanks!

In D75837#1947981, @silvas wrote:

Presumably yield should replace the linalg and loop yields? I would add std.yield in a separate patch that refactors the other dialects to use it as well.

Add test case

Add test case (commit id change on rebase)

Add test case

Address review comments.

Minor comment fix.