In D83004#2290157, @greened wrote:

In D83004#2287851, @dmajor wrote:

The expensive-checks bots have been red for several days. Could you please take a look or revert?

Working on it...

In D83004#2287851, @dmajor wrote:

The expensive-checks bots have been red for several days. Could you please take a look or revert?

Fix one inadvertent change to the original algorithm in update_test_checks.py. This line:

output_lines.append(input_line)

had an extra rstrip in it compared to the original. It was probably an innocuous change but fixing this makes the diff a bit cleaner.

In D83004#2278681, @arichardson wrote:

Maybe add another test that checks for the @__cxx_global_var_init() constructor function?
E.g. something like this:

int init_func(int arg) {
    return arg + 1;
}

int my_global = init_func(2);

Refactored to pull out most of the --include-generated-funcs processing into common.py.

Enhanced update_test_checks.py to honor --include-generated-funcs and added tests. For fun, did the same for update_llc_test_checks.py. The latter doesn't have complete coverage for all architectures because the machine outliner doesn't run on every backend. I didn't look further into this but we can do so later if desired.

In D83004#2274687, @jdoerfert wrote:

Added tests.

In D82995#2211280, @RKSimon wrote:

@greened Any luck with a test case?

In D83004#2168427, @jdoerfert wrote:

Due to D82995 I realized we should have a test of this in llvm/test/tools/UpdateTestChecks as well.

In D83004#2250897, @jdoerfert wrote:

reverse ping?

Updated to move the option into common.py. Also had to rework the output loop to account for differences between python 2 and 3.

In D82995#2137715, @jdoerfert wrote:

We need a test.

If it helps, OpenMP begin/end declare variant mangles the names with $, so do we for CUDA (sometimes).

In D83004#2134303, @greened wrote:

In D83004#2129362, @jdoerfert wrote:

This is great! Just a few days ago I added a TODO in one of the tests I created asking for this: D82722 :)

Fixed various bugs, added tests.

In D82995#2129326, @spatel wrote:

It would be good to check in a test example alongside this change, so we know it works. (And we'll know what we are losing if this has to be reverted for some reason.)

In D83004#2126792, @MaskRay wrote:

In D83004#2126719, @greened wrote:

Do we have any tests for these tools? I vaguely recall some discussion about tests but I couldn't find any. I'd like to add tests for this feature if the infrastructure is already there.

Yes. See clang/test/utils/update_cc_test_checks/ (thanks to @arichardson)

Do we have any tests for these tools? I vaguely recall some discussion about tests but I couldn't find any. I'd like to add tests for this feature if the infrastructure is already there.

Ping?

In D68230#2081496, @lebedev.ri wrote:

Ok, so if test-suite seems.like a good place for such tests, can this change proceed?

In D68230#2042289, @echristo wrote:

In general I have a pretty strong objection to end to end tests in the clang repository unless there's no other way to test them.

Can you give an idea of what you plan on testing with this?

Getting back to this. From the (very long!) thread on end-to-end testing, people are generally supportive of the idea. The main question is where tests should be located. That question is othogonal to this patch. We currently use the functionality of this patch downstream and I'm guessing others would also find it useful. Since the idea of end-to-end testing seems to be supported in the community, can we merge this patch and then have a follow-up discussion about where such tests should live upstream? Getting this merged now would allow others to experiment with/use the functionality as discussion about where to place end-to-end tests continues.

In D78565#1997372, @greened wrote:

In D78565#1996297, @SjoerdMeijer wrote:

Fair enough, perhaps the audience is too small here on llvm.org for this and this is too niche. In A-profile we have the same problem, so could the exercise for an A-core here, but can't spend time on that now, so will abandon this.

I would find this extremely valuable in the clang documentation. Not sure why this should be put on developer.arm.com as it is compiler-specific as wel as (sub-)target-specific.

In D78565#1996297, @SjoerdMeijer wrote:

Fair enough, perhaps the audience is too small here on llvm.org for this and this is too niche. In A-profile we have the same problem, so could the exercise for an A-core here, but can't spend time on that now, so will abandon this.

This appeared using a quite old version of LLVM with an out-of-tree target and I'm having trouble getting a valid testcase out that runs with the latest LLVM/bugpoint. The code in question is exacly the same in the old and current LLVM so the bug still exists currently but I haven't yet been able to generate a testcase. Thought I would at least put this up for review. I'm not entirely confident current master could get into this situation but it seems wise to guard against it.

In D71618#1789459, @rengolin wrote:

@jdoerfert @hfinkel should I consider Johannes' comment a LGTM? This has been waiting for two months now.

LGTM. I did something similar in my local copy that I was going to upstream but I added a switch for the user to choose where to place checks (declaration, definition or both). I only added the switch because I wasn't sure whether the changed behavior was intended. I'm fine just placing checks on definitions as I don't have a need to put them on declarations.

In D69088#1772141, @rjmccall wrote:

This is a major new language feature, and code review is probably not the right venue for reviewing it; there should be a thread on cfe-dev. My apologies if that's already been discussed and I missed it.

Updated to latest master.

Ping.

Ping. This is ready for another review.

Added comment explaining what a write-combining buffer is and one possibility of how to use this information.

Ok, I think I've addressed all the comments so this is ready for another review. Thanks!

LGTM. This is useful functionality.

This revision avoids the more general use of register scavenging, opting instead for a local register scavenger which can operate in backward mode, which doesn't need up-to-date liveness information. This seems safer to me, avoiding lots of potentially unknown pitfalls in the X86 backend.

This causes a couple of tests to fail due to the X86 backend not keeping liveness information up-to-date. My understanding is that the liveness bits should go away. Currently PEI uses RegisterScavenging::forward which is commented as being not preferred. The better option is to use RegisterScavenging::backward which doesn't need to rely on liveness information. However, PEI iterates through the basic block in a forward manner, so using RegisterScavenging::backward could be expensive.

This likely needs some work. I wanted to get it up early because it includes a rather significant change, the first use of register scavenging in the X86 backend as far as I'm aware. If there is a better way to do this I'm all ears.

LGTM.

In D68819#1704962, @jdoerfert wrote:

We can, or should, combine D68153 and this, either in one or two patches.

Does this subsume the goal of D68153? If so I am happy to abandon that revision. D68153 attempts to solve the problem of a CHECK-LABEL matching a function call instead of the start of a function definition. It looks like with --function-signature the CHECK-LABEL will include the arguments in the label pattern which should be enough to disambiguate it from a call to the function. Do I have that right?

we might say something like:

\return the number of write-combining buffers. A write-combining buffer is a per-core resource used for collecting writes to a particular cache line before further processing those writes using other parts of the memory subsystem.

In D68793#1704266, @hfinkel wrote:

How do you imagine that we'd use this? Do we need some kind of size to go along with this?

LGTM.

In D63614#1702176, @beanz wrote:

This patch causes lots of warning spews because it adds virtual methods to a class that doesn't have a virtual destructor.

In D67728#1686494, @greened wrote:

In D67728#1678378, @xbolva00 wrote:

I could mock-up a testcase but where does it go?

As you mentioned SCEV, you can add new/update test for SCEV. But I dont think this "step" is required.

So, that leaves me with a question. What else needs to be done before getting this committed?

In D68153#1689791, @RKSimon wrote:

Wouldn't this mean that every regeneration would see this change?

For some reason the commit did not auto-update this. Closing with commit r373912/a14ffc7eb741de4fd7484350d11947dea40991fd.

In D68272#1690823, @MaskRay wrote:

Can you give an example demonstrating the issue?

In D68230#1690948, @RKSimon wrote:

It then becomes a problem of maintenance - if a developer is just building/testing the llvm project for backend work are they responsible for keeping that test project passing? If not then it prevents us using CI bots to test and breaks are likely to linger for long lengths of time.

LGTM, thanks!

My thinking on this is now that we have the monorepo and that the monorepo will become the canonical source after the conference this month, it's much more feasible to create end-to-end tests. They could live in a subdirectory of the monorepo root. They wouldn't be largish things like in test-suite but rather more focused tests that ensure some behavior works in the context of the entire tool pipeline.

We also need to use the asm check generator so that LABEL expressions are generated correctly.

In D68230#1688429, @lebedev.ri wrote:

For the record, is this for the use in upstream clang tests?

In D68230#1688351, @xbolva00 wrote:

Can you show simple example? Is it something like

int foo(void) {
; CHECK: xor eax, eax
return 0;
}

Than great, +1

The more I think about this, the more I do think this should use from __future__ import print_function.

Should this use from __future__ import print_function to avoid someone accidentially printing arguments as a tuple?

LGTM.

In D68153#1688102, @RKSimon wrote:

Is there an existing test file in trunk that you can regenerate to the show the diff?