run tests once

bump? this should be an easy obvious bugfix

I would be supportive of a patch that added a TTI function that returned the contents of the target-attribute when it is present or the TargetMachine attributes when it is not.

I noticed the first 2 RUN statements in the file were accidentally identical, so I fixed one and reran the update_llc_test_checks.py to fix the tests

I was tracking back a recent ABI break (also failing now in gcc 12, so maybe this irregularity is intentional), and was concerned that this commit is observed to cause the platform ABI to change depending on the feature flags of the current compilation unit. Prior to this change, f16 was always treated as i16 for the purpose of the calling-convention (e.g. returned in %ax). But after this change, the ABI of the value is now inconsistent between compile units. I made a small change to one of the existing tests to show this. Note how the callq result was in %ax without this mattr flag, and in %xmm0 with this mattr flag added. But the function known as "identity.half" is external, and did not change between those two calls to the llvm.

bump?

bump?

Hm, I am actually not 100% certain, since I think that might be a current bug now that I think about it, which was caused by failing to expose this information about the TargetMachine. It looks some passes are adding flags there (e.g. +thumb/-thumb) with the implied assumption that they should be inheriting the global TargetMachine flags for anything left unspecified, while other passes (e.g. msan) are checking only the Function's flags, with the implied assumption that the configured TargetMachine does not specify target-features, and yet other passes (MIR/Codegen) are probably assuming that the "target-features" flag should entirely replace and override the TargetMachine's feature set. These assumptions seem currently incompatible.

It can matter in what ways the the "target-attribute" is different. Just grepping the source tree reveals there are several existing IR passes that depend on examining this function attribute, and are already altering their behavior based on the value. For example, it appears that the msan pass should be falling back to examining the TTI's notion of the target machine to locate the function arguments correctly, if I am reading the source comments here correctly:

TargetMachine is nearly always a required argument here, so clearly the analysis already does depend on the the TargetMachine. Sometimes the information we (aka JuliaLang) need is available in the "target-features" function attribute (which is always available), but we want to compare that against the TargetMachine's notion of the TargetFeatures during optimization. Those together are then nearly the complete set of arguments to the TargetMachine, so it seemed more logical to me to expose the TM directly, rather than separately exposing all of the information needed to reconstruct a copy of it. I don't think we should obfuscate this just for the sake of obfuscation, do you?

ping @fhahn

Seems extra complexity, and less desirable to lose test coverage, when it seems the option may already be unreliable and soon to be deprecated by https://reviews.llvm.org/D119383. @MaskRay is that accurate?

fix

this is adding a new restriction

Is that a bug in clang or in MSVC2019? We expected that CLANG_PLUGIN_SUPPORT=ON can expose problems with the Windows linker, and we add new tests here for verifying that configuration is functioning as expected. You might need to disable CLANG_PLUGIN_SUPPORT on that target?

Okay, this should work now for those cases! Sorry, I kept thinking it was clang/examples that was broken, and missed that it was clang/test that was failing, so I failed to see what I needed to fix there too, so that we were not trying to build the tests in configurations where the functionality is disabled.

add one more missing case to check

fixup

We are not going to start exporting plugin support if the user explicitly disabled it with CLANG_PLUGIN_SUPPORT=OFF, but why is it trying to read that directory at all, when it should be disallowed by this PR unless CLANG_PLUGIN_SUPPORT=ON

cleanup

Thanks, if you encounter more issues next week, let me know, and I will continue trying to adjust it to work for all config option combinations.

@cristian.adam Is this good now? You are blocking merging this, but I think it is ready to land, and I would like to not hold it up for other people if it is fixing their issues (and the issues you discovered too)

Fixed by D119199

Ah, this looks annoying: there are apparently two flags, CLANG_PLUGIN_SUPPORT and LLVM_ENABLE_PLUGINS, but the existing build for clang uses CLANG_PLUGIN_SUPPORT to turn off the build support and LLVM_ENABLE_PLUGINS to turn off the tests (you might not have noticed this existing issue since you turned off CLANG_ENABLE_STATIC_ANALYZER support, and CLANG_BUILD_EXAMPLES is off by default, which looks like it would disable all of the existing tests for this functionality). This might fix this particular test?

diff --git a/clang-tools-extra/test/CMakeLists.txt b/clang-tools-extra/test/CMakeLists.txt
index 9321457ae1a3..c98ec90a179b 100644
--- a/clang-tools-extra/test/CMakeLists.txt
+++ b/clang-tools-extra/test/CMakeLists.txt
@@ -17,7 +17,7 @@ string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} CLANG_TOOLS_DIR ${LLVM_RUN

It is a somewhat worthless test IMO, and might belong better in LLVM itself (where this functionality is defined), but there does not appear to be any other like it currently, and it was requested by a previous reviewer. Comparing to the code in LLVMTestingSupport, does this fix it for you:

diff --git a/clang-tools-extra/test/CMakeLists.txt b/clang-tools-extra/test/CMakeLists.txt
index 9321457ae1a3..17cc12473565 100644
--- a/clang-tools-extra/test/CMakeLists.txt
+++ b/clang-tools-extra/test/CMakeLists.txt
@@ -87,6 +87,15 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
       PLUGIN_TOOL clang-tidy
       DEPENDS clang-tidy-headers)

TargetTransformInfo does not currently have what I need, but if I can add it there, it would also satisfy my needs. I wanted it for an optimization pass (to pre-populate some codegen decisions earlier in the pipeline for better performance), but it looks like all current in-tree consumers are codegen-related.

Closed by rG84f137a590e7de25c4105303e5938c40566c2dfb

rebase

@fhahn You added these test failures recently, but they are working now. Does this look good to you to enable them?

I don't know the plan specifics, but I am just exposing an existing analysis pass to the command line that already has been converted, not changing it to define any new passes.

I decided it made the most sense to me to go with option 3, so this should be ready to land again.

• Reland "enable plugins for clang-tidy"
• fixup! Reland "enable plugins for clang-tidy": Disable the test if the user has disabled support for building it.

It looks like this is probably the first time that a test was written for a PLUGIN_TOOL, outside of the docs, so we are likely in new territory here :/

Yes, please push a revert so I can look later. Do you have a link to the buildbot configuration, so I can reproduce that?

@aaron.ballman I think I incorporated all of your feedback. Is this okay for me to merge to main? I would like to get it in before the feature branch.

arc error

address review feedback

Can you upload the full diff (git diff -U100000 or arc diff --update D116852) and fix the clang-format check?

Thanks! I will work on making those changes

run update_test_checks.py on these files too

The work had already pretty much been done before (for GVN), we just needed to
allow it to use that work here for NewGVN, and use bits directly rather than
bytes/8*8 for the output computation.

rebase onto LLVM main, allowing it to handle null values everywhere, and not just for non-integral pointers

There is clearly some more work to do to get the cmake file to be correct, but was hoping to check this looked like the direction you thought looked right for adding this test, since there isn't an obvious example to follow.

I couldn't think of an example where you'd want to make sure all of the assigned categories were related to the command being used, instead of showing the option if at least one was related to it. Currently it hides the option as soon as any don't match, whereas it seems like you'd want to hide the option only if all categories don't match.

update