Could you sync post https://github.com/llvm/llvm-project/commit/ad1d60c3befd606d6864b367f939238e50fb0f7e ?

A followup will work on plumbing more locations through

tablegen, which should greatly improve what we can do here.

Nice thanks

Nice (which reminds me I need to update the injection in another spot post https://github.com/llvm/llvm-project/commit/999fb7d0450a804f3eb0d16d2853e8667658f39e)

Not sure I follow why c-style is requirement? E.g., copyright in Python files is close to // per line

LGTM (and yay for usage of LITERAL ;-))

LG, thanks

Looks good/we can refine in follow ups, thanks!

(first sweep, mostly looks good, just some clarifications)

In D125051#3495776, @rriddle wrote:

In D125051#3495775, @mehdi_amini wrote:

they can get the same behavior by using the SameOperandsAndResultShape and SameOperandsAndResultElementType traits.

Should SameOperandsAndResultShape be renamed CompatibleOperandsAndResultShape?

Yeah, let me do that in a followup.

I'll let the argument about the location of this slide as I know you'll follow up ;-)

Good point about potentially expensive operations here, I wonder: why is/was this a canonicalization pattern rather than fold implementation?

Thanks, and yes with Marius 's change for doc includes (couple months back :-)) there isn't really reason to have multiple be generated website side. Being explicit here is better.

In D125427#3511309, @sebastian-ne wrote:

FWIW, there’s a tree-sitter grammar here: https://github.com/Flakebi/tree-sitter-tablegen

LGTM, could you add a before and after screenshot of a test? (Just for easy visual check)

(1) the operands defined by non-constant-like ops come first, followed by (2) block arguments, and these are followed by (3) the operands defined by constant-like ops.

I agree on breaking at once, too many breaking changes one after the other is painful (looking at JAX team needing to bump API number for small changes makes me feel bad). Perhaps we could roll https://gist.github.com/jpienaar/9d22b710db6122a9a41e8cd3d824a39e or something like it in here too (also breaking change but orthogonal to this one, but in similar spirit). Ripping off bandaid should result in one larger change but enable less breaking changes post. I do have preference for consistency with ODS and C++ here (it may also result in less breakages but that's not the reason why I prefer it). It puts convenience and usability in the hand of dialect authors. Now the signature can change if one changes the op def, but merely appending optional/default value attribute would not. But I might not be a "pythonic" person :-)

(I'm OOO, but randomly checking so my latency will be high)

Nice, thanks!

Makes sense to have some marker here, its a hole in the type description SGTM

Nice!

Nice, thanks

Quite a lot of red :-)

Thanks!

(was using the wrong formatter)

Nice, thanks!

Could we add simple smoke test for -d output? (Not golden of course)

Nice, thanks (I need to remove the hard coding on SameElement... there but that's not related here directly)

In D121279#3464917, @arnab-oss wrote:

Can you please provide the test case for which incorrect IR is generated?
I will provide the fix. Thanks in advance.

Conceptually Module had func as default dialect before too but having Builtin know of Func feels weird, so LGTM.

Nice, good point about the change to dump for invalid IR, this seems to provide the functionality of the previous change without needing a flag/API change and producing the prettier output in more cases. Of course the discussion still ongoing but looks like general improvement.

The cost of getting custom format output from generic output dump is rerunning through opt tool, the cost of crash is rerunning program (which can take multiple hours and take a lot of compute resources). I'd much rather it be uniform and safe and avoid needing flag plumbed through.

Thanks

In D123474#3448889, @NathanielMcVicar wrote:

This is causing failures on the Windows builbot: https://lab.llvm.org/buildbot/#/builders/13/builds/19626/steps/6/logs/stdio
I believe you simply need #include <string> to make MSVC happy.

C:\PROGRA~2\MICROS~3\2019\COMMUN~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe  /nologo /TP -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=1 -DMLIR_ROCM_CONVERSIONS_ENABLED=1 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:\buildbot\mlir-x64-windows-ninja\build\tools\mlir\lib\Tools\mlir-pdll-lsp-server -IC:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\lib\Tools\mlir-pdll-lsp-server -IC:\buildbot\mlir-x64-windows-ninja\build\include -IC:\buildbot\mlir-x64-windows-ninja\llvm-project\llvm\include -IC:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\include -IC:\buildbot\mlir-x64-windows-ninja\build\tools\mlir\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2  /EHs-c- /GR- -UNDEBUG -std:c++14 /showIncludes /Fotools\mlir\lib\Tools\mlir-pdll-lsp-server\CMakeFiles\MLIRPdllLspServerLib.dir\PDLLServer.cpp.obj /Fdtools\mlir\lib\Tools\mlir-pdll-lsp-server\CMakeFiles\MLIRPdllLspServerLib.dir\MLIRPdllLspServerLib.pdb /FS -c C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\lib\Tools\mlir-pdll-lsp-server\PDLLServer.cpp
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\lib\Tools\mlir-pdll-lsp-server\PDLLServer.h(32): error C2039: 'string': is not a member of 'std'
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\vector(24): note: see declaration of 'std'
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\lib\Tools\mlir-pdll-lsp-server\PDLLServer.h(32): error C2065: 'string': undeclared identifier
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\lib\Tools\mlir-pdll-lsp-server\PDLLServer.h(32): error C2923: 'std::vector': 'string' is not a valid template type argument for parameter '_Ty'
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\lib\Tools\mlir-pdll-lsp-server\PDLLServer.h(32): note: see declaration of 'string'

Nice :-)

SG, thanks for the explanation

In D123360#3439676, @silvas wrote:

Nice :)

Thanks! (I was going to start with doc update, but that requires coffee :-))

I might even suggest the error emission from not finding LSP should be logged (flagged in some way) rather than popup given the extension is the combination of LSP & syntax highlighting. And I'm reading these files more than I traverse via LSP assistance, so in fresh client/machine without lsp built the extension is still useful and the missing LSP warning interrupts my flow before I even tried to use any LSP functionality (and I may not need any at all). But that's just a nit.

Nice, thanks that SGTM

Improved Inclusivity to make it beginner-friendly

In D122770#3417918, @mehdi_amini wrote:

Chose this as my expectation is that extern constant would be used as part of workflow that manages state, but the moment one outputs to textual format you have left that.

I have strong concerns with this: even if there is some external state, the IR should round-trip as much as possible..

You should be able to test this with a module with an arith.constant

In D122692#3421019, @rriddle wrote:

In D122692#3421016, @rriddle wrote:

In D122692#3421001, @Mogball wrote:

So there are at least 2 significant changes to behaviour

1. The folder actually sorts commutative ops, but this doesn't trigger the greedy rewriter to run another iteration because tryToFold might still return failure. If -my-pass only has one pattern commutative(a, b) -> foo but the input IR is commutative(b, a), the pattern doesn't match because the folder will re-order the operands but the rewriter will exit.

Was this kind of awkwardly relying on the fact that we always did 2 iterations (given that we refolded existing constants)?

LG, I'm just not sure how much the optionality/null is getting you here vs extra paths created.

Nice, this was a pain point for folks diff'ing across passes, thanks!