- User Since
- Mar 31 2020, 3:33 AM (112 w, 3 d)
Jul 12 2021
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.
Jul 9 2021
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.
Jun 27 2021
Jun 26 2021
This revision slipped under my radar and the new policy change was addressed by https://reviews.llvm.org/D101083. Closing.
Mar 30 2021
Check for the CMake version before setting the policy. Unknown policies cause errors during the CMake configuration phase.
Feb 14 2021
Nov 12 2020
Add the missing tokens to the documentation and add question mark parsing functions
Nov 11 2020
Add tests and rebase
Nov 6 2020
Oct 26 2020
Oct 15 2020
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.
Jul 1 2020
Yes, sorry about blocking the revision. Note that there is still a clang-format issue in one of the files.
Jun 26 2020
Jun 25 2020
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.
Jun 24 2020
Not sure why clang-tidy complains about EnumsGenTest.h.inc here.
Jun 23 2020
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.
Jun 18 2020
nit: You may want to update the title before landing it
Jun 5 2020
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.
Jun 2 2020
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.
Jun 1 2020
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.
May 31 2020
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?
May 28 2020
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
May 22 2020
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.
May 21 2020
@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.
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.
May 20 2020
Also propagate memory operands when folding non-MOV instructions. Add a test case.
May 19 2020
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.
May 18 2020
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.
May 16 2020
Also propagate store memory operands during call frame optimization.
May 14 2020
May 5 2020
Apr 28 2020
nit: The commit message looks a little convoluted. Maybe you could simplify/structure it a little bit.
Apr 24 2020
It is just a cleanup. I ran into an issue related to this missing check in an out-of-tree project.