Page MenuHomePhabricator

[UpdateCCTestChecks] Include generated functions if asked
ClosedPublic

Authored by greened on Jul 1 2020, 3:30 PM.

Details

Summary

Add the --include-generated-funcs option to update_cc_test_checks.py so that any
functions created by the compiler that don't exist in the source will also be
checked.

We need to maintain the output order of generated function checks so that
CHECK-LABEL works properly. To do so, maintain a list of functions output for
each prefix in the order they are output. Use this list to output checks for
generated functions in the proper order.

Diff Detail

Event Timeline

greened created this revision.Jul 1 2020, 3:30 PM

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.

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.

Yes. See clang/test/utils/update_cc_test_checks/ (thanks to @arichardson)

Ah, didn't think to look under clang since the tool lives under llvm. Thanks!

This is great! Just a few days ago I added a TODO in one of the tests I created asking for this: D82722 :)

Will this work for all test scripts?

Will this make the prefix_exclusions logic obsolete?

llvm/utils/update_cc_test_checks.py
149

I think this should go into common.py (after D78618). I would also make this the default but OK.

greened updated this revision to Diff 275831.Jul 6 2020, 2:34 PM

Fixed various bugs, added tests.

This now has two modes because generated function output can't be ordered with respect to source file functions. When clang generates functions it can sometimes output original source functions in a different (non-source) order so checks can't be placed next to their definitions in the source file.

I don't particularly like this mode dichotomy but unifying it would necessitate updating a whole lot of clang tests.

Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2020, 2:34 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
greened marked an inline comment as done.Jul 6 2020, 2:38 PM
greened added inline comments.
llvm/utils/update_cc_test_checks.py
149

Yes I suppose it should in case opt and friends generate functions. I hadn't considered that use-case.

While I would like to make it default unfortunately it would require updating a bunch of the existing clang tests which doesn't seem too friendly. See the patch update comment for details.

This is great! Just a few days ago I added a TODO in one of the tests I created asking for this: D82722 :)

Glad to help!

Will this work for all test scripts?

Obviously right now it's only enabled for clang but it should be straightforward to add to the other test scripts. Al the infrastructure is there, the various drivers just have to use it.

Will this make the prefix_exclusions logic obsolete?

Yeah, I think it might.

I don't particularly like this mode dichotomy but unifying it would necessitate updating a whole lot of clang tests.

Axtually that's not strictly true, It would just change the tests singificantly when they are updated for some other reason. Whether that is reasonable is something we should discuss.

greened marked an inline comment as done.Jul 6 2020, 6:21 PM
greened added inline comments.
llvm/utils/update_cc_test_checks.py
149

Just realized it wouldn't necessarily require regeneration of tests, it would just cause regenerated tests to change a lot when they are eventually regenerated. We should discuss as to whether that's acceptable. I think for now this should be non-default to at least get the functionality in without disturbing existing users and then we can discuss a separate change to make it default.

It's also possible we could change how clang orders functions. I discovered there's a difference in clang 10 vs. 11 in the order functions are output when OpenMP outlining happens. clang 10 seems to preserve the source order of functions and clang 11 does not. Perhaps that needs to be fixed as I don't know whether that change was intentional or not.

jdoerfert added inline comments.Jul 6 2020, 7:15 PM
llvm/utils/update_cc_test_checks.py
149

Best case, without the option the original behavior is preserved. Is that not the case?

greened marked an inline comment as done.Jul 7 2020, 8:01 AM
greened added inline comments.
llvm/utils/update_cc_test_checks.py
149

That's right. I was referring to making this behavior default. If we do that, we could clean up the script code a bit but it would mean clang tests would change pretty dramatically when they are regenerated. If we fix the clang output, the test changes wouldn't be so dramatic.

The way clang is behaving now, I would expect any tests that use -fopenmp, have multiple functions with OpenMP regions and use function prototypes for some of those functions would break given clang's reordering of function definitions. Perhaps we don't have any tests like that though.

jdoerfert added inline comments.Jul 7 2020, 5:48 PM
llvm/utils/update_cc_test_checks.py
149

We (almost) do not have OpenMP tests with autogenerated test lines. Partially, because we do not test new functions. I would really like this to be available for OpenMP, both in _cc_ and IR tests. If people can opt out of this, especially if the default is "off", the ordering is not a problem (IMHO). With UTC_ARGS we also remember the choice so I really don't see the downside to this being in the common part for all scripts.

greened marked an inline comment as done.Jul 22 2020, 11:33 AM
greened added inline comments.
llvm/utils/update_cc_test_checks.py
149

I agree it can be in the common part. I'll move it there and will leave it off by default so it doesn't disturb existing tests.

greened updated this revision to Diff 279932.Jul 22 2020, 1:26 PM

Updated to move the option into common.py. Also had to rework the output loop to account for differences between python 2 and 3.

jdoerfert accepted this revision.Jul 22 2020, 5:27 PM

LGTM. Thanks for the generalization. We'll add update_test support next for the opt case, e.g., for D83635.

llvm/utils/update_cc_test_checks.py
361

This is all unfortunate but at least for OpenMP not easily changeable. Let's go with this.

This revision is now accepted and ready to land.Jul 22 2020, 5:27 PM

Due to D82995 I realized we should have a test of this in llvm/test/tools/UpdateTestChecks as well.

reverse ping?

reverse ping?

Heh. Getting back to this now.

Due to D82995 I realized we should have a test of this in llvm/test/tools/UpdateTestChecks as well.

Do you mean a test for recognizing the option in common.py? I'm not sure what you're asking for here.

Due to D82995 I realized we should have a test of this in llvm/test/tools/UpdateTestChecks as well.

Do you mean a test for recognizing the option in common.py? I'm not sure what you're asking for here.

The function is recognized in common.py, right? Can't you run the update_test_check.py it with that option now? If so, we could have a test for it, using, for example, hot-cold splitting or attributor with the -attributor-allow-deep-wrappers and a linkonce_odr function that is called.

Do you mean a test for recognizing the option in common.py? I'm not sure what you're asking for here.

The function is recognized in common.py, right? Can't you run the update_test_check.py it with that option now? If so, we could have a test for it, using, for example, hot-cold splitting or attributor with the -attributor-allow-deep-wrappers and a linkonce_odr function that is called.

Makes sense. I'll work on it.

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);
llvm/utils/update_cc_test_checks.py
254–258

This should avoid a few update() calls. Not that performance really matters here.

arichardson added inline comments.Sep 17 2020, 1:50 AM
llvm/utils/update_cc_test_checks.py
361

How difficult would it be to only use this behaviour if the normal one fails?

If we encounter a function that's not in the source code we just append the check lines for the generate function where we last added checks. And if we then see a function where the order doesn't match the source order fall back to just emitting all checks at the end of the file?

greened updated this revision to Diff 292588.Sep 17 2020, 12:25 PM

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.

This works for me. @arichardson you want to continue the review or should we take this and if needed improve upon it in subsequent patches?

greened updated this revision to Diff 292623.Sep 17 2020, 2:40 PM

Refactored to pull out most of the --include-generated-funcs processing into common.py.

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);

Can you explain what this would test beyond the provided OpenMP tests? Remember, the tests are for the update tool itself, not to test that the compiler is doing the right thing.

greened added inline comments.Sep 17 2020, 2:52 PM
llvm/utils/update_cc_test_checks.py
254–258

This should avoid a few update() calls. Not that performance really matters here.

Wouldn't it iterate over the prefix list twice? In any event, I was trying to follow existing style and wanted to rewrite as little existing code as possible.

361

How difficult would it be to only use this behaviour if the normal one fails?

If we encounter a function that's not in the source code we just append the check lines for the generate function where we last added checks. And if we then see a function where the order doesn't match the source order fall back to just emitting all checks at the end of the file?

The problem is that this loop is driven by lines as they appear in the source file and uses a dictionary to look up the tool output for that function. Because of this checks will *always* be placed in the source before each function and there is no way to tell if the checks you're adding are from out-of-order tool output. Changing this would be a complete rewrite of the algorithm, which I wanted to avoid for this enhancement. Of course we can always rewrite it later but I think that is best done as a separate change.

The other two tools use a similar algorithm. In fact it *may* be possible to further refactor this code to share the check output driver. But again, that involves changing existing code and is best left to a separate change I think.

greened updated this revision to Diff 292630.Sep 17 2020, 2:57 PM

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.

I think this approach seems fine, just a few very minor comments inlines.

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);

Can you explain what this would test beyond the provided OpenMP tests? Remember, the tests are for the update tool itself, not to test that the compiler is doing the right thing.

It wouldn't add anything in terms of test coverage, I was just thinking of an example where the generated function is (at least currently) always inserted at the same location.

llvm/utils/UpdateTestChecks/common.py
608

I found this function a bit difficult to parse due to not knowing what finder does. Maybe something like get_arg_to_check ?

llvm/utils/update_cc_test_checks.py
294

This is currently always true, but I guess adding it doesn't do any harm.

301

Since we're writing the output to a C/C++ file we need to use // comments.

361

Yes, I think the current approach is fine since it will only affect new tests that add --include-generated-funcs. In the future I think it would be nice if this flag could be passed to existing tests that don't have any generated functions and keep the output stable (other than the UTC_ARGS).

llvm/utils/update_llc_test_checks.py
14

seems unused?

This revision was landed with ongoing or failed builds.Sep 18 2020, 4:35 AM
This revision was automatically updated to reflect the committed changes.
greened marked an inline comment as done.
greened marked 4 inline comments as done.Sep 18 2020, 4:38 AM
thakis added inline comments.Sep 18 2020, 7:02 AM
llvm/utils/UpdateTestChecks/common.py
645

LLVM still supports Python 2.7. This breaks devs and bots still using 2.7. Please fix :)

dmajor added a subscriber: dmajor.Mon, Sep 21, 7:48 AM

The expensive-checks bots have been red for several days. Could you please take a look or revert?

riscv_generated_funcs.test:

*** Bad machine code: MBB exits via unconditional fall-through but ends with a barrier instruction! ***
- function:    check_boundaries
- basic block: %bb.4  (0x5653e22035b8)
LLVM ERROR: Found 1 machine code errors.

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/9171/steps/test-check-all/logs/FAIL%3A%20LLVM%3A%3Ariscv_generated_funcs.test

The expensive-checks bots have been red for several days. Could you please take a look or revert?

Working on it...

The expensive-checks bots have been red for several days. Could you please take a look or revert?

Working on it...

I've removed this test as it seems to expose a bug in MachineOutliner for RISCV. Since that's not at all related to this change it made the most sense to me to just remove the test.