Right, seems reasonable enough until we figure out something better. Thanks for the clarification and the pointers!

If LinalgNamedStructuredOps.yaml is generated by a script, would it be possible to remove it from the repository entirely and regenerate it during the build process? I feel that checking out a generated file into the repository may lead to some inconsistencies between the script and the generated version upstream if someone forgets to commit an updated version of LinalgNamedStructuredOps.yaml after each update to dump_oplib.py.

I don't have any specific use case. I think it may still be worth simplifying the function if it ever happens to be emitted by something.

This revision slipped under my radar and the new policy change was addressed by https://reviews.llvm.org/D101083. Closing.

Check for the CMake version before setting the policy. Unknown policies cause errors during the CMake configuration phase.

LGTM. Since @aartbik and @mehdi_amini seem to agree, I think you could land this patch.

Add the missing tokens to the documentation and add question mark parsing functions

Add tests and rebase

This looks like the same as my old patch (https://reviews.llvm.org/D80161). There are additional details available on the commit's Phab page. To put it in a nutshell, some codes can trigger a very large amount of calls to the aliasing check (see the repro provided by @nemanjai: https://pastebin.com/tRtTQdSa), which results in a very large increase in compilation time.

Yes, sorry about blocking the revision. Note that there is still a clang-format issue in one of the files.

I'm wondering how hard it would be to have a linter check for TableGen files that reports whether there are lines longer than 80 characters. This could be useful for such situations: I had to change some locations in OpBase.td that went unnoticed during earlier reviews (e.g. the constBuilderCall for I32EnumAttr).

Make sure that OpBase.td is wrapped at 80 characters.

Not sure why clang-tidy complains about EnumsGenTest.h.inc here.

Yes, it's still in the mlir namespace. Moving it out of mlir would probably help, but I don't think there is enough coverage in it right now to have a meaningful impact. The issue fixed in this patch would not have been detected for example, since there are no custom passes in examples/standalone yet.

nit: You may want to update the title before landing it

Maybe you could move to a slightly different directory structure and follow what other dialects are doing. Using a top-level CMakeLists.txt like

# lib/Dialect/Shape/CMakeLists.txt
add_subdirectory(IR)

and then add the dialect library generation directives in lib/Dialect/Shape/IR?
I think it would make sense to follow the same conventions as, say, the Linalg dialect.

It looks like there are clang-format errors (https://results.llvm-merge-guard.org/amd64_debian_testing_clang-1651/summary.html). Can you format your changes using git-clang-format HEAD^ and update the diff? This would allow us to have a clean build.

Regarding the print and dump methods, I have no strong opinions. We may as well keep them as-is for now.
Simplex.cpp is apparently still using // for top-level comments.
Regarding the change to SmallVector instead of std::vector, it would probably make sense to use ArrayRef<T> for function parameters since it makes the API more flexible. If you need to mutate the parameter then I would suggest using SmallVectorImpl<T> instead of specifying the size of the internal buffer in the parameter type.

I added quite a lot of inline comments. To avoid redundancies for some of them, here are more general ones that apply to all the files:

• Don't use Doxygen tags in your comments.
• Use /// for top-level comments (e.g. outside of functions).
• Prefer SmallVector over std::vector since the former can potentially be more efficient for small sizes.
• Fraction and Matrix look like they are only ever used with int64_t elements. Is there a need for them to be template classes?

LGTM

LGTM too, after adding the test case. Thanks!

@ftynse This would suggest an update to this part of the docs then, right? https://llvm.org/docs/Phabricator.html#reviewing-code-with-phabricator

@nemanjai The changes were reverted by commit 65cd2c7a8015577fea15c861f41d2e4b5768961f.

Sure. I didn't realize it would cause such an overhead. If I remember correctly, some MachineInstr instances sometimes have duplicate memory operands for the same load or store.

@efriedma Thanks for your feedback! I had to make some minor adjustments to ARM test cases. If it's still good for you, I will land the patch.

Update FileCheck directives in

llvm/test/CodeGen/ARM/cortex-a57-misched-vldm-wrback.ll
llvm/test/CodeGen/ARM/cortex-a57-misched-vstm-wrback.ll
llvm/test/CodeGen/ARM/cortex-a57-misched-vstm.ll

to avoid matching the new instruction scheduler debug output.

In D80161#2044571, @efriedma wrote:

On ARM specifically, operations don't usually more than one memoperand, with the exception of load store paired/multiple. So yes, I can see it would be hard to trigger outside of scheduling.

Maybe we could add some debug output to the scheduler showing when it does/does not add a dependency, and check that. So it would be checking scheduling, but not the final schedule.

Add some debug output to the instruction sheduler to signal wether or not a chain dependency has been added between two given instructions. This change allows us to properly test the effect of handling multiple memory operands in alias queries on instruction scheduling.

Also propagate memory operands when folding non-MOV instructions. Add a test case.

In D80161#2043591, @Kayjukh wrote:

In D80161#2042626, @efriedma wrote:

I'd like to see an MIR test that shows the aliasing check actually working correctly.

I will have to find something that fits in a test case and covers this case. Working on it!

Simplify the method's control flow by wrapping the old dependency checking code in a helper lambda and calling it for each pair of memory operands.

In D80161#2042626, @efriedma wrote:

I'd like to see an MIR test that shows the aliasing check actually working correctly.

Update instruction ordering in failing test cases.

Add a test case.

Split the call frame optimization patch from the ISel DAG postprocessing patch.

LGTM after fixing the use of curly braces.

Also propagate store memory operands during call frame optimization.

LGTM, thanks!

nit: The commit message looks a little convoluted. Maybe you could simplify/structure it a little bit.
Otherwise, LGTM.

It is just a cleanup. I ran into an issue related to this missing check in an out-of-tree project.

In D77643#1967192, @LukasSommerTu wrote:

In D77643#1967140, @Kayjukh wrote:

I think it would make sense to propagate the changes to mlir-opt as well.

The changes to mlir-opt.cpp propagate the change of the file-management to mlir-opt. Or did you refer to something else?