[UpdateCCTestChecks] Allow asm in output with --allow-asm

[greened]

Summary

It is useful to have update_cc_test_checks.py generate asm checks for end-to-end testing. Add --allow-asm to generate test checks even if --emit-llvm doesn't appear on the clang command line.

[xbolva00]

Can you show simple example? Is it something like

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

Than great, +1

[greened]

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

Yes, that's exactly right. Take some C/C++ source and match against expected assembly from clang. Editing the result is generally required to make the checks less brittle but that's true of all these check generation scripts.

[lebedev.ri]

In D68230#1688415, @greened wrote:

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

Yes, that's exactly right. Take some C/C++ source and match against expected assembly from clang. Editing the result is generally required to make the checks less brittle but that's true of all these check generation scripts.

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

[greened]

In D68230#1688429, @lebedev.ri wrote:

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

Not currently since AFAIK we don't have any upstream end-to-end tests. But perhaps we should. test-suite exists but I don't know if that uses generation scripts.

[MaskRay]

llvm/utils/update_cc_test_checks.py

136

The comment is here because there was petty strong objection to have .cc->.s tests checked in in the repository. For end-to-end tests that are personal uses, I think it is fine. If you want to extend that, you'll have to seek for more opinions.

[greened]

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

[greened]

llvm/utils/update_cc_test_checks.py

136

I'm not sure what you're saying. Are you saying this is ok to go in as long as upstream tests don't use it without permissions or that this change itself needs more reviewers? If the latter, do you have suggestions? You and Dávid are the only people who have worked on this script so I wasn't sure who else to include.

[greened]

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.

[xbolva00]

Yeah, this could be useful.

Mans clang tests which sadly depend on -O1/2/3 pipeline could be moved to somewhere else..

[MaskRay]

llvm/utils/update_cc_test_checks.py

136

I don't have good suggestions right now but you can probably raise the topic on the mailing list about end-to-end tests.

[RKSimon]

In D68230#1689718, @greened wrote:

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.

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.

[greened]

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.

I've been uncomfortable with the notion of people working on pieces of the compiler but not testing the entire compiler. As a data point, does gcc development work that way? I know there was a long discussion about the monorepo proposal and people wanting to just take a slice of the compiler and only worry about that but I don't think that's feasible. Back when LLVM was just an optimizer and code generator one didn't have to worry about other pieces but now we have an integrated toolset and it seems rather unsustainable to let people make changes without testing.

If we're not doing end-to-end testing then that is a large hole in our testing infrastructure and thus reduces our confidence in the final product. If we are going to do end-to-end testing then we need to do it and have everyone be responsible for it.

I'll start a conversation on the mailing list. In the meantime, is there any reason not to merge this change? Just because someone *could* generate asm tests from C source doesn't mean the project has to accept the test.

[greened]

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.

[echristo]

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?

[greened]

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?

The thread has a lot of details but I'll attempt to summarize. Basically, there is nothing that currently tests that a full run through the clang pipeline actualy generates expected code. This is important for things that aren't sufficient to test in isolation. For example, we can test that the prefetching pass produces prefetches in the IR and we can test that IR with prefetches turns into prefetch instructions but we currently can't test that clang/LLVM maintains all of the proper information and conditions needed for prefetches to make it from the prefetch pass all the way through codegen and into prefetch instructions. There are many reasons there might be problems: clang emitting "unexpected" IR, LLVM doing optimisations that remove/obscure prefetch opportunities and so on. The problem with individual unit tests is that the tests set an environment that might not be what clang actually produces and when that unit test produces might not make it all the way through to the end.

That said, I have no particular objection to putting such tests somewhere other than clang. That question is orthogonal to this patch and indeed my initial proposal was to put the tests in a top-level test directory. This patch only *allows* update_cc_test_checks.py to generate asm. It doesn't mandate its use. Many people have expressed support for this functionality and we have found it invaluable in our testing, where we definitely see breakage in the clang pipeline not covered by existing unit tests. How we incorporate such tests upstream is a separate question.

Keep in mind that end-to-end tests are *not* meant to be a first line of defense. They are meant to catch things that slip through unit testing. One suggestion is to put such tests in test-suite and I would be open to that.

[echristo]

In D68230#2051241, @greened wrote:

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?

The thread has a lot of details but I'll attempt to summarize. Basically, there is nothing that currently tests that a full run through the clang pipeline actualy generates expected code. This is important for things that aren't sufficient to test in isolation. For example, we can test that the prefetching pass produces prefetches in the IR and we can test that IR with prefetches turns into prefetch instructions but we currently can't test that clang/LLVM maintains all of the proper information and conditions needed for prefetches to make it from the prefetch pass all the way through codegen and into prefetch instructions. There are many reasons there might be problems: clang emitting "unexpected" IR, LLVM doing optimisations that remove/obscure prefetch opportunities and so on. The problem with individual unit tests is that the tests set an environment that might not be what clang actually produces and when that unit test produces might not make it all the way through to the end.

That said, I have no particular objection to putting such tests somewhere other than clang. That question is orthogonal to this patch and indeed my initial proposal was to put the tests in a top-level test directory. This patch only *allows* update_cc_test_checks.py to generate asm. It doesn't mandate its use. Many people have expressed support for this functionality and we have found it invaluable in our testing, where we definitely see breakage in the clang pipeline not covered by existing unit tests. How we incorporate such tests upstream is a separate question.

Keep in mind that end-to-end tests are *not* meant to be a first line of defense. They are meant to catch things that slip through unit testing. One suggestion is to put such tests in test-suite and I would be open to that.

I think the llvm test-suite would be a good place to put this sort of thing. We did that when we wanted to test some other end to end for C++ simd work and I think it's probably the best way. Would also be interested in figuring out how to try to get the best testing we can without having any end to end tests. Sometimes it happens because we don't have sufficient separation, but that should be in the absolute minority.

Thanks!

[greened]

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

[lebedev.ri]

In D68230#2081308, @greened wrote:

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

Like this was already disscussed (i think?) in some -dev thread, how would that work?
If such tests are in test-suite, why would they need update_cc_test_checks.py?
If they need update_cc_test_checks.py, then now everyone also needs to run those tests before commit?
And what happens if they break? Just silently regenerate them?
Also, test-suite is not in monorepo, so you can't update those tests at the same time as making llvm change.

TLDR: i don't think the burden of proof has been provided here.

[greened]

In D68230#2081496, @lebedev.ri wrote:

Like this was already disscussed (i think?) in some -dev thread, how would that work?

I must have missed that message.

If such tests are in test-suite, why would they need update_cc_test_checks.py?

For the same reason any other tests needs it. The compiler changes and sometimes we need to update tests.

If they need update_cc_test_checks.py, then now everyone also needs to run those tests before commit?

Not at all. The discussion seemed to settle on the bots running test-suite to be sufficient. If one of them fails, then yes, the author would have to address the test, just like any other test in test-suite.

And what happens if they break? Just silently regenerate them?

What do we do for such tests in clang/LLVM today? It's exactly the same situation. I am of the opinion that we do far too much blind regeneration of tests and I have a bunch of follow-on commits that will hopefully improve that situation by making update_*_tests.py more featureful and able to produce smaller, more robust tests.

Also, test-suite is not in monorepo, so you can't update those tests at the same time as making llvm change.

Yes, that's a consequence of putting them in test-suite. My personal preference would be to put them in the monorepo but the community seemed to decide that's not the right place, at least for now.

TLDR: i don't think the burden of proof has been provided here.

I'm not sure what "proof" you're looking for. There seems to be fairly wide agreement that asm-style tests are useful. The controversey, if any, is about where they are placed.

[greened]

Ping?

[lebedev.ri]

In D68230#2126392, @greened wrote:

Ping?

I'm not sure this is stuck due to the lack of review.
I'm not seeing much excitement here for the expected usage of this
(regardless of whether these tests go into clang or test-suite),
nor do i recall that https://lists.llvm.org/pipermail/llvm-dev/2019-October/135739.html resolved positively :/

llvm/utils/update_cc_test_checks.py

def config():
   parser.add_argument(
       '--functions', nargs='+', help='A list of function name regexes. '
       'If specified, update CHECK lines for functions matching at least one regex')
   parser.add_argument(
       '--x86_extra_scrub', action='store_true',
       help='Use more regex for x86 matching to reduce diffs between various subtargets')
   parser.add_argument('-u', '--update-only', action='store_true',
                       help='Only update test if it was already autogened')
+  parser.add_argument('--allow-asm', action='store_true',
+                      help='Accept machine asm in test checks')
   parser.add_argument('tests', nargs='+')
   args = parser.parse_args()
   args.clang_args = shlex.split(args.clang_args or '')
 
   if args.clang is None:
     if args.llvm_bin is None:
       args.clang = 'clang'
     else:

 def get_function_body(args, filename, clang_args, prefixes, triple_in_cmd, func_dict):
   # TODO Clean up duplication of asm/common build_function_body_dictionary
   # Invoke external tool and extract function bodies.
   raw_tool_output = common.invoke_tool(args.clang, clang_args, filename)
   if '-emit-llvm' in clang_args:
     common.build_function_body_dictionary(
             common.OPT_FUNCTION_RE, common.scrub_body, [],
             raw_tool_output, prefixes, func_dict, args.verbose)
+  elif args.allow_asm:
+    triple = triple_in_cmd
+    if not triple:
+      march_in_cmd = None
+      m = common.MARCH_ARG_RE.search('{}'.format(clang_args))
+      if m:
+        march_in_cmd = m.groups()[0]
+      triple = asm.get_triple_from_march(march_in_cmd)
+    asm.build_function_body_dictionary_for_triple(args, raw_tool_output,
+                                                  triple, prefixes, func_dict)
   else:
     print('The clang command line should include -emit-llvm as asm tests '
           'are discouraged in Clang testsuite.', file=sys.stderr)
     sys.exit(1)
 
 
 def main():
   args = config()
   script_name = os.path.basename(__file__)
   autogenerated_note = (ADVERT + 'utils/' + script_name)
for idx, line in enumerate(input_lines):
           # we simply require the spelling name to appear on the line
           # to exclude functions from other files.
           if mangled in added or spell not in line:
             continue
           if args.functions is None or any(re.search(regex, spell) for regex in args.functions):
             if added:
               output_lines.append('//')
             added.add(mangled)
+            if '-emit-llvm' in clang_args:
               common.add_ir_checks(output_lines, '//', run_list, func_dict, mangled)
+            else:
+              asm.add_asm_checks(output_lines, '//', run_list, func_dict, mangled)
       output_lines.append(line.rstrip('\n'))
 
     # Update the test file.
     with open(filename, 'w') as f:
       for line in output_lines:
         f.write(line + '\n')
 
   return 0
 
 
 if __name__ == '__main__':
   sys.exit(main())