In D116832#3303166, @pengfei wrote:

I think my concerns still exist, even though you provide one more option. The author selects between cannot foresee what in their tests will be affected by future commit. And the --scrub-reg deps loses too much information.

I guess to me this just brings feature parity with update_test_checks.py to update_llc_test_checks.py. Both can now scrub variable/register names.

In D116832#3301796, @RKSimon wrote:

I'm still struggling to understand real use cases here - you spoke about just wanting to match specific instructions (nontemporal load/store) - but wasn't that handled by D117694 --filter and --filter-out?

I guess I'm getting worried that these scripts are experiencing feature creep for use cases that are barely a real issue.

Ping. I think the latest changes should address some of the concerns about applicability. The scrub-regs=deps option creates a very generic kind of check.

Updated to change to a --scrub-reg option with a value of "names" or "deps." --scrub-reg=names will create FileCheck variables as before while --scrub-reg=deps will replace with non-capturing regular expressions.

Updated with bug fixes. Posting so that future visitors will see the change mentioned in the Differential Revision field of the commit message.

Ping.

Ping. Can we merge this?

Fixed a brown paper bag bug (filter-out was not working).

Fixed typo.

In D116832#3257235, @pengfei wrote:

What I mainly wanted to demonstrate is there exists such a test generated for other purpose. For a scheduling patch, the unexpected changes of other use and def of %R1 and %R2 on an unrelated test are confusing.

; CHECK-NEXT:    add %[[GPR_1:(%r[0-9]+)]], %[[GPR_2:(%r[0-9]+)]] <== These two lines won't be changed, that's fine.
; CHECK-NEXT:    add %[[GPR_1:(%r[0-9]+)]], %[[GPR_3:(%r[0-9]+)]]
; ... ...
; CHECK-NEXT:    other_use_def %[[GPR_2]] <== These lines will be changed, that's bad.
; CHECK-NEXT:    other_use_def %[[GPR_3]] <== Same here.

D117694 is the revision adding the filtering capability. We could improve this register scrubbing for more filtered cases but I'd prefer to do that as a later enhancement.

In D116832#3253630, @pengfei wrote:

A patch that only exchanges the order of the first add to

; CHECK-NEXT:    add %R0, %R2
; CHECK-NEXT:    add %R0, %R1

Then, the other use and def of %R1 and %R2 will remain unchanged.
But if you are numbering the register by order, all the other %R1 and %R2 will be changed while the expected change won't be shown. This is rather confusing.

In D116832#3252510, @lebedev.ri wrote:

Sorry if this has been asked before, but i think the main question here is: what's the target audience here?

In D116832#3245756, @pengfei wrote:

Although the new regexp looks much concise, you still haven't solved the second question @hvdijk raised.

Fixed non-clobber register def regexp for x86. It was using lookbehind instead of negative lookahead for vector mnemonics.

Clarified comments.

In D116832#3240865, @RKSimon wrote:

It might be better if you do this for a target other than x86 - if generating arm/aarch64 codegen tests with the update scripts are a concern for those teams, using it there makes more sense, plus they have simpler register sets.

Made the FileCheck variable names more generic, to limit diffs between test updates. Names are now based on register class with a uniquifying count that tracks redefinitions of registers so that changes in register def-use changes can be seen.

I think I can also shorten up the regexps by being smarter about what kind of register is used in the asm. I'll work on that as well. It should make things much more readable and will flag changes in the register class, which is probably desirable.

All of the examples you have provided show non-register-changes in the same tests, so the tests would still need re-generating. After re-generating, would all the register name updates still not be part of the diff on exactly the same lines?

IIUC, the point of this change is not to alter the behavior for options that have a cl::init, but rather to alter behavior for options that _don't_ have a default provided. Right now, those options don't get reset when ResetAllOptionOccurrences is called. With this patch, options _without_ cl::init should get reset to the default-constructed value of the option type, and options with cl::init get initialized to the default value.

In D116832#3236435, @pengfei wrote:

We have a huge issue with codegen churn causing pages of testcase changes in proposed merges and very little review is done on them.

Can you show an example?

In D116832#3236187, @pengfei wrote:

Agreed with Simon, this is a disaster for doing code review.

In D116832#3235656, @RKSimon wrote:

Have you examples of actual test checks that you've had problems with that this is a answer to?

In D116832#3234787, @sebastian-ne wrote:

The implementation looks fine to me, but I’m not sure what the goal is.

What is the motivation for this change? I can argue for maintaining the current behavior., If I pass a cl::init to an option and call ResetAllOptionOccurrences I would expect that cl::init value to be used to reset the option. There are compiler flows that rely on that behavior.

We do need an X86 test for this.

In D88595#2744776, @dancgr wrote:

@paulwalker-arm @sdesmalen I will be picking up this work, I will study this patch and see if I have any questions. Thanks!

Updated to main, implemented some suggested changes.

This is still on my TODO list but got pushed aside for a bit due to other issues. I expect to be back on this in the next couple of weeks.

Not my area of expertise but looks fine if the comments from others are addressed.

I don't really have the expertise to approve this. The patch looks fine from a readability standpoint.

This is pretty far afield from my areas of expertise so I can't really comment on the overall approach of this patch. From a purely mechanical/style point of view this seems fine to me.

In D68911#2024813, @sanwou01 wrote:

I'm running SPEC CPU intrate for this patch as well as this patch in combination with D78880.

Make clang-tidy (more?) happy.

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.