Page MenuHomePhabricator

Allow update_test_checks.py to not scrub names
ClosedPublic

Authored by greened on Sep 26 2019, 4:53 AM.

Details

Summary

Add a --preserve-names option to tell the script not to replace IR names.
Sometimes tests want those names. For example if a test is looking for a
modification to an existing instruction we'll want to keep the names to
identify the instruction.

Diff Detail

Event Timeline

greened created this revision.Sep 26 2019, 4:53 AM

I'm not sure this is a good idea.

I'm not sure this is a good idea.

Can you explain a bit more? The use-case is testing a transformation that simply updates an instruction, such as adding metadata. The assumption is that the generated test will be edited for brevity (as the script comments indicate) and that the user knows that names could change. Many times such tests will never change names because they are trivial, only running a single pass that doesn't add or delete any Values.

I'm not sure this is a good idea.

I also don't see the value in this. With autogenerated checks, the name of the instructions shouldn't be required to tell if the test is getting the correct behaviour and preserving the names makes test checks much more fragile.

I also don't see the value in this. With autogenerated checks, the name of the instructions shouldn't be required to tell if the test is getting the correct behaviour and preserving the names makes test checks much more fragile.

So here's the situation I ran into. I have hundreds of tests to update. I don't want to match all of the IR, just selected instructions. The current behavior of rewriting the operands with FileCheck variable names makes it difficult to delete parts of the IR. Maintaining the original names makes it easy to delete IR.

Perhaps there is another way to handle situations like this. I am open to ideas!

Perhaps there is another way to handle situations like this. I am open to ideas!

One idea would be to have a mode that replaces names not with FileCheck variable references but with a simple regexp like `"%[^[:space:]]+". That's what I end up doing for the tests in question anyway. Does that seem better?

I also don't see the value in this. With autogenerated checks, the name of the instructions shouldn't be required to tell if the test is getting the correct behaviour and preserving the names makes test checks much more fragile.

So here's the situation I ran into. I have hundreds of tests to update. I don't want to match all of the IR, just selected instructions. The current behavior of rewriting the operands with FileCheck variable names makes it difficult to delete parts of the IR. Maintaining the original names makes it easy to delete IR.

This is not what an autogenerated test is. An auto-generated test includes the final state, and updating it is simply running the update script. We do not want to encourage customization of the output or manual workflows.

This is not what an autogenerated test is. An auto-generated test includes the final state, and updating it is simply running the update script. We do not want to encourage customization of the output or manual workflows.

Well, that's pretty much the opposite of what the script says:

A common pattern is to have the script insert complete checking of every
instruction. Then, edit it down to only check the relevant instructions.
The script is designed to make adding checks to a test case fast, it is *not*
designed to be authoratitive about what constitutes a good test!
reames accepted this revision.Oct 3 2019, 4:16 PM

Well, that's pretty much the opposite of what the script says:

I stand corrected. Given that, the patch looks reasonable.

Not a use case I particular want to encourage, but I take your point about existing documented behaviour.

This revision is now accepted and ready to land.Oct 3 2019, 4:16 PM
greened closed this revision.Oct 7 2019, 5:52 PM

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

I had update_cc_test_checks.py give me a Python traceback just now which I think must be to do with this commit:

Traceback (most recent call last):
  File "llvm/utils/update_cc_test_checks.py", line 267, in <module>
    sys.exit(main())
  File "llvm/utils/update_cc_test_checks.py", line 255, in main
    common.add_ir_checks(output_lines, '//', run_list, func_dict, mangled)
TypeError: add_ir_checks() missing 1 required positional argument: 'preserve_names'

The new parameter to add_ir_checks is mandatory, but it's only been added to the call in update_test_checks.py, and not to the one in update_cc_test_checks.py.