Add a few type-related Matchers
- bitIntType
- constantMatrixType
- dependentAddressSpaceType
- dependentBitIntType
- dependentSizedMatrixType
- dependentVectorType
- hasTypeForDecl
- objcTypeParamDecl
- objcTypeParamType
- pipeType
- vectorType
Differential D158872
[clang][ASTMatchers] Add a few type-related Matchers danix800 on Aug 25 2023, 11:54 AM. Authored by
Details Add a few type-related Matchers
Diff Detail
Event TimelineComment Actions LGTM.
Comment Actions Thanks for the reviewing. Ping @aaron.ballman, could you please take a look at this revision? Comment Actions Are these matchers going to be used in-tree (by clang-tidy, or something else)? We typically do not add new AST matches until there's a need for them because the AST matchers have a pretty big impact on build times of Clang itself.
Comment Actions They are used in ASTImporter testcases as shown in https://reviews.llvm.org/D158948. Though this might not be a strong reason to ASTImporter is more urgent since we still lack support for some of the AST nodes so I considered adding them batchly and started with If not acceptable, I'm OK with it. We can still test importing with other ways. Matchers are not mandatory. EDIT: If acceptable, tools like clang-query can benefit from these matchers too, easy for Comment Actions Pulling in a few more folks for extra opinions in case I'm off-base, but I think a good approach for right now is to add these matches to the unit test file directly in the AST importer patch and to hold off on landing this one. Then you still get the test coverage you need in the unit tests, but the extra compilation time is limited to just one .cpp file and not everything including ASTMatchers.h. As we find a need for the matchers, we can lift them from the unit test into the public matchers. It's not that there's no utility to these (there is!), it's more just the balancing act between compile times and utility. Comment Actions This is a sad state of affairs :-( A few ideas:
Comment Actions Agreed, but it's also not a new state of affairs; we've held this position for as long as we've had AST matchers. (For reference, ASTMatchers.h creates so many symbols we needed to enable /bigobj just to be able to generate an object file that could represent it -- you use that option when you have > 65k addressable sections in an object file!)
Agreed. :-(
The problem is the pile of templates that is used to generate the DSL and used for dynamic matchers by clang-query. IIRC, we looked into reducing the overhead and eventually punted on it, some background can be found here: https://reviews.llvm.org/D84038 when we finally decided to enable /bigobj everywhere. I know we did a pass to split ASTMatchers.h up so we now have ASTMatchersInternal.h, so I think we might have done about as much extraction as we can (though it'd be worth seeing if there's more we can do now that we've got access to C++17 in source; I think we were still on C++11 when we last did this). Ideally, I would love to table generate more of the AST matchers from source such as https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/TypeNodes.td and https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DeclNodes.td so that we automatically get all of the type and declaration matchers (at least the top-level matchers) without any extra maintenance overhead to keep them in sync. But I've always had the impression this idea was dead in the water because of overhead. I suspect moving things around into multiple headers won't help all that much (and it will likely cause confusion as to what header file to put any given AST matcher into) because Clang itself will still need to include all of those headers (and thus will still hit all the compile time and object size problems). Comment Actions Thanks for the history, I wasn't aware this was such a big deal for such a long time! Thanks, I'll take a look and see if C++17 gives us new tools, but seems likely it's just too complicated.
The thing I think is possible is to have a header that defines DeclMatcher, BoundNodes, anything etc without including all the concrete matchers. This is the header that ASTMatchFinder.h should include. (At the moment we have some insane include chains like Analysis/FlowSensitive/DataflowAnalysis.h => MatchSwitch.h (for TransferState) => ASTMatchers.h` (for anything etc). Thus the main header of an ~unrelated library exports all matcher types).
That would be great. For compile times, I'd expect we could include fewer matchers into fewer TUs - very little of clang should be using most of the matchers, and the sum of TU compile times is usually somewhat more important than the max. Comment Actions I did some testing on my machine (host compiler clang-14), and I'm not sure the specifics justify the level of caution here: ASTMatchers.h parses in 6.5s (with clang-check) The bug referenced is about object size. The connection with matchers seems to be that matcher tests had their section limit increased, before this applied to the whole project. (If compilation of files like NodeMatchersTest.cpp are significant build bottlenecks, we'd need to address that, but *those* would be fine to split up in unprincipled ways, so I'm not too concerned) Comment Actions Thank you for doing some research on this!
Out of curiosity, are you testing on Windows with MSVC? My understanding on build time performance impacts was that it's debug builds with MSVC that get hit especially hard. (I can test that setup but I could use some help figuring out how you tested compile times so I can do a similar test instead of something totally different from what you tried.) It would be fantastic if the build time issues were not a problem!
FWIW, we had to increase the limits on various files until we finally broke down and did it project-wide. So it wasn't just limited to testing files having the issue; I think the big problem was templated symbols from instantiations. But as you say, if it's pay-for-what-you-use, that's pretty reasonable. CC @PiotrZSL @LegalizeAdulthood @carlosgalvezp as folks who work on clang-tidy, which uses AST matchers all over the place. They may also have some good insights or preferences here. Comment Actions I'm not :-( I don't have a windows machine, and the VM I used to have has bitrotted beyond repair (not sure how useful benchmarks from there would be anyway). This is with clang on linux, just running time clang-check-14 include/clang/ASTMatchers/ASTMatchers.h a few times. I guess the equivalent of clang-check would be /Zs, not sure the best way to feed it the right flags... Comment Actions Thanks for the testing. I can do the windows part with MSVC cl.exe.
$ uname -a MINGW64_NT-10.0-20348 WIN-LH267487PAC 3.3.3-341.x86_64 2022-01-17 11:45 UTC x86_64 Msys $ echo $SHELL /usr/bin/bash $ cat test.ps1 & "C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\bin\Hostx64\x64\cl.exe" /permissive- /MP /we"4238" /GS /Zc:preprocessor /W4 /wd"4141" /wd"4146" /wd"4244" /wd"4267" /wd"4291" /wd"4351" /wd"4456" /wd"4457" /wd"4458" /wd"4459" /wd"4503" /wd"4624" /wd"4722" /wd"4100" /wd"4127" /wd"4512" /wd"4505" /wd"4610" /wd"4510" /wd"4702" /wd"4245" /wd"4706" /wd"4310" /wd"4701" /wd"4703" /wd"4389" /wd"4611" /wd"4805" /wd"4204" /wd"4577" /wd"4091" /wd"4592" /wd"4319" /wd"4709" /wd"5105" /wd"4324" /Zc:wchar_t /I"C:\Users\Administrator\source\repos\llvm-project\build-main-vs2022\tools\clang\lib\ASTMatchers" /I"C:\Users\Administrator\source\repos\llvm-project\clang\lib\ASTMatchers" /I"C:\Users\Administrator\source\repos\llvm-project\clang\include" /I"C:\Users\Administrator\source\repos\llvm-project\build-main-vs2022\tools\clang\include" /I"C:\Users\Administrator\source\repos\llvm-project\build-main-vs2022\include" /I"C:\Users\Administrator\source\repos\llvm-project\llvm\include" /Gm- /O2 /Ob2 /Zc:inline /fp:precise /U"NDEBUG" /D "_UNICODE" /D "UNICODE" /D "WIN32" /D "_WINDOWS" /D "_GLIBCXX_ASSERTIONS" /D "_LIBCPP_ENABLE_ASSERTIONS" /D "_CRT_SECURE_NO_DEPRECATE" /D "_CRT_SECURE_NO_WARNINGS" /D "_CRT_NONSTDC_NO_DEPRECATE" /D "_CRT_NONSTDC_NO_WARNINGS" /D "_SCL_SECURE_NO_DEPRECATE" /D "_SCL_SECURE_NO_WARNINGS" /D "__STDC_CONSTANT_MACROS" /D "__STDC_FORMAT_MACROS" /D "__STDC_LIMIT_MACROS" /D CMAKE_INTDIR=\"Release\" /errorReport:prompt /WX- /Zc:forScope /GR /Gd /Oi /MD /std:c++17 /EHsc /nologo /Fp"obj.clangASTMatchers.dir\Release\obj.clangASTMatchers.pch" /diagnostics:column /Zc:__cplusplus /bigobj -w14062 /Gw /I"C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\include" /I"C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\atlmfc\include" /I"C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Auxiliary\VS\include" /I"C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\ucrt" /I"C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Auxiliary\VS\UnitTest\include" /I"C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um" /I"C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\shared" /I"C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\winrt" /I"C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\cppwinrt" /c ..\clang\include\clang\ASTMatchers\ASTMatchers.h /TP $ time powershell ./test.ps1 ASTMatchers.h real 0m3.143s user 0m0.015s sys 0m0.000s EDIT: Obj size is incorrect, sorry! # incorrect $ du -h ASTMatchers.obj 8.0K ASTMatchers.obj $ du -b ASTMatchers.obj 5283 ASTMatchers.obj About 5.3k. Comment Actions From a mine perspective when it comes to clang-tidy:
As for matchers in this change, except maybe hasTypeForDecl, I do not think that I will be using them. |
Maybe: "Matches a fixed int type of a specified bitwidth."