Page MenuHomePhabricator

[Utils] Allow update_test_checks to check function arguments
ClosedPublic

Authored by jdoerfert on Oct 10 2019, 10:31 AM.

Details

Summary

This adds a switch to the update_test_checks that triggers arguments to
be present in the check line. If not set, the behavior should be the
same as before.

Diff Detail

Event Timeline

jdoerfert created this revision.Oct 10 2019, 10:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 10:31 AM
Herald added a subscriber: bollu. · View Herald Transcript

This doesn't change the default, right?

xbolva00 added inline comments.Oct 10 2019, 11:54 AM
llvm/utils/UpdateTestChecks/common.py
140
if 'args' in m.groupdict():
        if record_args:
             args = m.group('args').strip()
        else:
             args = '('

Include personality, fix versioning based on different args, tested on D68766

This doesn't change the default, right?

It shouldn't no. I tested it on some formatted files but I'm unsure if there are other interactions I don't know about.

@xbolva00 I don't understand but I updated the version, please take a look

greened added a comment.EditedOct 10 2019, 1:23 PM

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?

EDIT: Actually, I don't think it will completely work if there is a recursive call to, say, foo that passes the same arguments through. In that case the call will look exactly like the function signature. The same is true if an unrelated function makes a call to foo with values that just happen to be named the same as foo's arguments.

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?

EDIT: Actually, I don't think it will completely work if there is a recursive call to, say, foo that passes the same arguments through. In that case the call will look exactly like the function signature. The same is true if an unrelated function makes a call to foo with values that just happen to be named the same as foo's arguments.

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

jdoerfert updated this revision to Diff 224520.Oct 10 2019, 6:16 PM

Lessons learned by running it on all argument promotion files: D68766

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

Sounds good. Do you think D68153 should operate under the --function-signature flag, under a different flag or always include define in the pattern (meaning all tests will change when the tool is re-run)?

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

Sounds good. Do you think D68153 should operate under the --function-signature flag, under a different flag or always include define in the pattern (meaning all tests will change when the tool is re-run)?

I think D68153 is a small enough change and at the same time helpful enough to run it always. Adding function signature is not for everyone so I added the flag. I can put it under the flag if people think it should live there though.

jdoerfert marked an inline comment as done.Mon, Oct 28, 2:43 PM

ping

greened accepted this revision.Wed, Oct 30, 2:25 PM

LGTM. This is useful functionality.

This revision is now accepted and ready to land.Wed, Oct 30, 2:25 PM
This revision was automatically updated to reflect the committed changes.

This seems to have broke update_llc_test_checks.py. Is there an easy fix?

Traceback (most recent call last):
  File "./llvm/utils/update_llc_test_checks.py", line 214, in <module>
    main()
  File "./llvm/utils/update_llc_test_checks.py", line 155, in main
    triple, prefixes, func_dict)
  File "/home/dave/s/l/llvm/utils/UpdateTestChecks/asm.py", line 357, in build_function_body_dictionary_for_triple
    func_dict, args.verbose)
TypeError: build_function_body_dictionary() missing 1 required positional argument: 'record_args'

Just general note:

Since in recent time there were multiple patches for update scripts, should we consider proper test suite for these scripts?

It also seems to break update_cc_test_checks.py

nikic added a subscriber: nikic.Thu, Oct 31, 2:15 PM

Even if --function-signature is not present, this generates spurious diffs:

-; CHECK-LABEL: @test1(
+; CHECK-LABEL: define {{[^@]+}}@test1(
nikic added a comment.Thu, Oct 31, 2:28 PM

Ugh, I now see that this is actually intentional. I think any changes to the default behavior of update_tests_checks should be discussed on llvm-dev in the future. These kinds of changes are quite disruptive and in this case imho don't pay for themselves.

Ugh, I now see that this is actually intentional. I think any changes to the default behavior of update_tests_checks should be discussed on llvm-dev in the future. These kinds of changes are quite disruptive and in this case imho don't pay for themselves.

It prevents us from accidentally matching the wrong thing, see D68153. We can (now), if we really want to, put that change under the flag as well. I really don't think it should be under a flag. While I see the cost of having new define {{[^@]+}} in upcoming diffs, it will actually improve the match quality. Now if that is worth it is obviously up for debate.

FWIW, D68850 will also change "the default" but only if the default would not have allowed to use the test script in the first place.

Ugh, I now see that this is actually intentional. I think any changes to the default behavior of update_tests_checks should be discussed on llvm-dev in the future. These kinds of changes are quite disruptive and in this case imho don't pay for themselves.

+1

I even asked about this:

This doesn't change the default, right?

It shouldn't no. I tested it on some formatted files but I'm unsure if there are other interactions I don't know about.

And was under impression that it was still the case as per

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

Sounds good. Do you think D68153 should operate under the --function-signature flag, under a different flag or always include define in the pattern (meaning all tests will change when the tool is re-run)?

I think D68153 is a small enough change and at the same time helpful enough to run it always. Adding function signature is not for everyone so I added the flag. I can put it under the flag if people think it should live there though.

@jdoerfert is this really fully intentional?

It also seems to break update_cc_test_checks.py

Yes, the breakage of the other update scripts was fixed in 4de09e0f4460.

Just general note:

Since in recent time there were multiple patches for update scripts, should we consider proper test suite for these scripts?

yes, please.

I said it before (in some thread) but I think actual improvements to the update scripts should not be delayed or hidden behind flags even if they cause some churn. We can always rerun the script on all files in a separate commit. This actually helps to discover other problems that sneak in over time anyway. As an example I rerun this on test/Analysis in D69687.

spatel added a comment.Fri, Nov 1, 7:48 AM

Here's a current example of the trade-off:
D59710

I agree that adding the 'define...' glob to the label check is an improvement, but at the same time, many of us use the scripts almost exclusively on single function regression tests (no calls)...which is probably why we never encountered the motivating problem for this change.

As I understand it, there's currently no way to disable the 'define' glob. So I either have the burden of doing separate pre-commits to update hundreds of test files or ask reviewers to look past that noise. We want to push people towards the better future, but it would be more friendly to offer this feature with an opt-out. That is, default to the new way, but let people disable the 'define' generation to update on their own terms/as needed.

Can the script rather somehow detect and warn for mentioned problematic cases than making churn? It can suggest “Problem detected. Use flag —XXX to emit precise checks”.

We should be careful how often we change default behaviour and if such thing is really needed.
Somebody in the future may again change the script -> new churn -> productivity decreases..

Or detect if no issue and emit old checks. No churn too.

Here's a current example of the trade-off:
D59710

I agree that adding the 'define...' glob to the label check is an improvement, but at the same time, many of us use the scripts almost exclusively on single function regression tests (no calls)...which is probably why we never encountered the motivating problem for this change.

As I understand it, there's currently no way to disable the 'define' glob. So I either have the burden of doing separate pre-commits to update hundreds of test files or ask reviewers to look past that noise. We want to push people towards the better future, but it would be more friendly to offer this feature with an opt-out. That is, default to the new way, but let people disable the 'define' generation to update on their own terms/as needed.

The combination of D69701 and D69719 will remove the unwanted effects but allow proper support in the future. If we get them in before your commit the churn goes away.

spatel added a comment.Fri, Nov 1, 1:07 PM

Here's a current example of the trade-off:
D59710

I agree that adding the 'define...' glob to the label check is an improvement, but at the same time, many of us use the scripts almost exclusively on single function regression tests (no calls)...which is probably why we never encountered the motivating problem for this change.

As I understand it, there's currently no way to disable the 'define' glob. So I either have the burden of doing separate pre-commits to update hundreds of test files or ask reviewers to look past that noise. We want to push people towards the better future, but it would be more friendly to offer this feature with an opt-out. That is, default to the new way, but let people disable the 'define' generation to update on their own terms/as needed.

The combination of D69701 and D69719 will remove the unwanted effects but allow proper support in the future. If we get them in before your commit the churn goes away.

Thanks! I haven't followed all of the script patches, but things are quiet once again in the files that I've updated just now.