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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/utils/UpdateTestChecks/common.py | ||
---|---|---|
140 | if 'args' in m.groupdict(): if record_args: args = m.group('args').strip() else: args = '(' |
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
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.
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.
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?
Even if --function-signature is not present, this generates spurious diffs:
-; CHECK-LABEL: @test1( +; CHECK-LABEL: define {{[^@]+}}@test1(
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.
+1
I even asked about this:
And was under impression that it was still the case as per
@jdoerfert is this really fully intentional?
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.
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..
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.