This is an archive of the discontinued LLVM Phabricator instance.

[utils] Fix UpdateTestChecks case where 2 runs differ for last label
ClosedPublic

Authored by mtrofin on Dec 10 2020, 4:42 PM.

Details

Summary

Two RUN lines produce outputs that, each, have some common parts and
some different parts. The common parts are checked under label A. The
differing parts are associated to a function and checked under labels B
and C, respectivelly.
When build_function_body_dictionary is called for the first RUN line, it
will attribute the function body to labels A and C. When the second RUN
is passed to build_function_body_dictionary, it sees that the function
body under A is different from what it has. If in this second RUN line,
A were at the end of the prefixes list, A's body is still kept
associated with the first run's function.

When we output the function body (i.e. add_checks), we stop after
emitting for the first prefix matching that function. So we end up with
the wrong function body (first RUN's A-association).

There is no reason to special-case the last label in the prefixes list,
and the fix is to always clear a label association if we find a RUN line
where the body is different.

Diff Detail

Event Timeline

mtrofin created this revision.Dec 10 2020, 4:42 PM
mtrofin requested review of this revision.Dec 10 2020, 4:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2020, 4:42 PM

Add initial author to have a review.

llvm/utils/UpdateTestChecks/common.py
296

I didn't dig into it too much, but the code looks to me that if all prefixes for the func don't meet the condition (already iterated to the last one), it reports warnings to user.
It seems you just removed the warnings instead of fixing the problem. So users hardly be aware of there's real conflict on their prefixes lists on RUN command line.

mtrofin marked an inline comment as done.Dec 10 2020, 8:50 PM
mtrofin added inline comments.
llvm/utils/UpdateTestChecks/common.py
296

I'm not sure what the original intent was there, but I'm not sure it serves a real reason anymore. See for example the "common-label-different-bodies-2.ll" test case, which would fail today (i.e. it'd produce the wrong outcome, and warn) - can't see why that would be desirable. Maybe I'm missing something?

pengfei added inline comments.Dec 10 2020, 9:24 PM
llvm/utils/UpdateTestChecks/common.py
296

I did an experiment: if you change both prefixes of the RUNs in your tests to the same one, you will find it doesn't generate checks for them and doesn't report any warning. I guess that's should be the original intent here.

mtrofin marked 2 inline comments as done.Dec 10 2020, 10:04 PM
mtrofin added inline comments.
llvm/utils/UpdateTestChecks/common.py
296

Ah, thanks! Like so (picking on the -2 file):

; RUN: llc < %s -mtriple=i686-unknown-linux-gnu -mattr=+sse2 | FileCheck %s --check-prefixes=A
; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s --check-prefixes=A

Got it. Indeed, the proposed behavior doesn't warn and doesn't generate an output for A; and the old behavior was generating incorrect asserts and outputing a warning.

This seems to end up as an unused prefixes case - (or am I holding a hammer and everything is a nail?). I suppose we can handle it in the tool after all the RUN lines are processed, but there are a few users of build_function_body_dictionary, so it'd be best to wrap the func_dict population; or, we can let it silently fail, and then FileCheck will complain quickly thereafter about duplicate prefixes.

Happy to do the wrapper in the tools, especially if this is something that happens frequently?

mtrofin updated this revision to Diff 311359.Dec 11 2020, 7:27 PM
mtrofin marked an inline comment as done.

added check for when a prefix has conflicts for all functions

Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2020, 7:27 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

What's the difference with the existing code? It looks to me that you just brought the warning out of loop, right?

llvm/utils/update_analyze_test_checks.py
128 ↗(On Diff #311359)

redundant space.

129 ↗(On Diff #311359)

Can we move these warn to common.py?

llvm/utils/update_cc_test_checks.py
269 ↗(On Diff #311359)

redundant space.

llvm/utils/update_test_checks.py
120 ↗(On Diff #311359)

redundant space.

mtrofin marked an inline comment as done.Dec 14 2020, 7:33 AM

What's the difference with the existing code? It looks to me that you just brought the warning out of loop, right?

Oh true! we can just do the check in build_function_body_dictionary_for_triple at the end.

I'll leave the additional tests, though, more tests never hurt.

mtrofin updated this revision to Diff 311593.Dec 14 2020, 7:34 AM
mtrofin marked 3 inline comments as done.

fix

pengfei accepted this revision.Dec 14 2020, 9:34 PM

LGTM. Thanks.
update_test_prefix.py assumes the conflicting output. You may need to change the expection of it as well.

This revision is now accepted and ready to land.Dec 14 2020, 9:34 PM

LGTM. Thanks.
update_test_prefix.py assumes the conflicting output. You may need to change the expection of it as well.

oh yes - made it check the new warning. Thanks!

mtrofin updated this revision to Diff 311896.Dec 15 2020, 7:16 AM

update_test_prefix.py change

This revision was landed with ongoing or failed builds.Dec 15 2020, 7:30 AM
This revision was automatically updated to reflect the committed changes.
mtrofin added inline comments.Dec 15 2020, 9:27 AM
llvm/utils/update_analyze_test_checks.py
129 ↗(On Diff #311359)

Come to think of it, maybe moving it to common.py was not quite ideal:

  • once a warning is issued in the middle of the list of RUN lines, it'll just be re-issued next time around; we could warn and exit, but that'd be annoying if there's another failure later in the RUN list.
  • if we do it at the end, we can additionally distinguish the case where a prefix has an empty dict of funcs associated with it -> and the warning would be "there are unused prefixes - please remove %s'. This is more discoverable than llvm-lit -a, and also than relying on the user knowing to run uplate_test_prefix.

I think that the benefits (discoverability, better, more concise warnings) make the extra 2-3 lines worth it; and we already have tests for these other tools - wdyt?

pengfei added inline comments.Dec 15 2020, 9:21 PM
llvm/utils/update_analyze_test_checks.py
129 ↗(On Diff #311359)

Make sence for me. Thanks for continuing to improve it.

jdoerfert reopened this revision.Jan 14 2021, 3:17 PM
jdoerfert added a subscriber: jdoerfert.

I think this caused a lot of problems for the Attributor tests. I get the "conflict output" warning all the time now :(

These two UpdateTestChecks tests also generate the warning. I would think that they haven't before.

LLVM :: tools/UpdateTestChecks/update_test_checks/check_attrs.test
LLVM :: tools/UpdateTestChecks/update_test_checks/prefix-never-matches.test

I suspect the --check-attributes flag effect are not taken into account somewhere but I might be wrong.

This revision is now accepted and ready to land.Jan 14 2021, 3:17 PM

I think this caused a lot of problems for the Attributor tests. I get the "conflict output" warning all the time now :(

These two UpdateTestChecks tests also generate the warning. I would think that they haven't before.

LLVM :: tools/UpdateTestChecks/update_test_checks/check_attrs.test
LLVM :: tools/UpdateTestChecks/update_test_checks/prefix-never-matches.test

I suspect the --check-attributes flag effect are not taken into account somewhere but I might be wrong.

The warnings are correct, but their purpose is to inform. For example, take check_attrs. The third and fourth opt run produce different function attributes from those produced by the first two, so the function is different insofar as update_test_prefix is concerned. The warnings are there to indicate that the listed prefixes end up not being used. But, that's 'by design' for the attributor tests, IIRC.

If the warnings are noise for you, we could add a flag to quiet them down, which you'd flip when regenerating attributor tests?

I think this caused a lot of problems for the Attributor tests. I get the "conflict output" warning all the time now :(

These two UpdateTestChecks tests also generate the warning. I would think that they haven't before.

LLVM :: tools/UpdateTestChecks/update_test_checks/check_attrs.test
LLVM :: tools/UpdateTestChecks/update_test_checks/prefix-never-matches.test

I suspect the --check-attributes flag effect are not taken into account somewhere but I might be wrong.

The warnings are correct, but their purpose is to inform. For example, take check_attrs. The third and fourth opt run produce different function attributes from those produced by the first two, so the function is different insofar as update_test_prefix is concerned. The warnings are there to indicate that the listed prefixes end up not being used. But, that's 'by design' for the attributor tests, IIRC.

If the warnings are noise for you, we could add a flag to quiet them down, which you'd flip when regenerating attributor tests?

TBH, before, the warnings meant there is a problem that needs fixing. Now they mean, there might be one or not. So, depending on your setup it's just noise while it was pure signal before.
I'm not sure how this is more helpful. What is the use case where this way of warning helps?

I think this caused a lot of problems for the Attributor tests. I get the "conflict output" warning all the time now :(

These two UpdateTestChecks tests also generate the warning. I would think that they haven't before.

LLVM :: tools/UpdateTestChecks/update_test_checks/check_attrs.test
LLVM :: tools/UpdateTestChecks/update_test_checks/prefix-never-matches.test

I suspect the --check-attributes flag effect are not taken into account somewhere but I might be wrong.

The warnings are correct, but their purpose is to inform. For example, take check_attrs. The third and fourth opt run produce different function attributes from those produced by the first two, so the function is different insofar as update_test_prefix is concerned. The warnings are there to indicate that the listed prefixes end up not being used. But, that's 'by design' for the attributor tests, IIRC.

If the warnings are noise for you, we could add a flag to quiet them down, which you'd flip when regenerating attributor tests?

TBH, before, the warnings meant there is a problem that needs fixing. Now they mean, there might be one or not. So, depending on your setup it's just noise while it was pure signal before.
I'm not sure how this is more helpful. What is the use case where this way of warning helps?

For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)

I think this caused a lot of problems for the Attributor tests. I get the "conflict output" warning all the time now :(

These two UpdateTestChecks tests also generate the warning. I would think that they haven't before.

LLVM :: tools/UpdateTestChecks/update_test_checks/check_attrs.test
LLVM :: tools/UpdateTestChecks/update_test_checks/prefix-never-matches.test

I suspect the --check-attributes flag effect are not taken into account somewhere but I might be wrong.

The warnings are correct, but their purpose is to inform. For example, take check_attrs. The third and fourth opt run produce different function attributes from those produced by the first two, so the function is different insofar as update_test_prefix is concerned. The warnings are there to indicate that the listed prefixes end up not being used. But, that's 'by design' for the attributor tests, IIRC.

If the warnings are noise for you, we could add a flag to quiet them down, which you'd flip when regenerating attributor tests?

TBH, before, the warnings meant there is a problem that needs fixing. Now they mean, there might be one or not. So, depending on your setup it's just noise while it was pure signal before.
I'm not sure how this is more helpful. What is the use case where this way of warning helps?

For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)

Should not we check for that flag in the RUN line then and only warn for unused prefixes when it is set. If there is no prefix we should obviously always warn.

I think this caused a lot of problems for the Attributor tests. I get the "conflict output" warning all the time now :(

These two UpdateTestChecks tests also generate the warning. I would think that they haven't before.

LLVM :: tools/UpdateTestChecks/update_test_checks/check_attrs.test
LLVM :: tools/UpdateTestChecks/update_test_checks/prefix-never-matches.test

I suspect the --check-attributes flag effect are not taken into account somewhere but I might be wrong.

The warnings are correct, but their purpose is to inform. For example, take check_attrs. The third and fourth opt run produce different function attributes from those produced by the first two, so the function is different insofar as update_test_prefix is concerned. The warnings are there to indicate that the listed prefixes end up not being used. But, that's 'by design' for the attributor tests, IIRC.

If the warnings are noise for you, we could add a flag to quiet them down, which you'd flip when regenerating attributor tests?

TBH, before, the warnings meant there is a problem that needs fixing. Now they mean, there might be one or not. So, depending on your setup it's just noise while it was pure signal before.
I'm not sure how this is more helpful. What is the use case where this way of warning helps?

For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)

Should not we check for that flag in the RUN line then and only warn for unused prefixes when it is set. If there is no prefix we should obviously always warn.

That's a good idea. Probably we'd need to also make sure that the unused prefixes are all on RUN lines with --allow-unused-prefixes=true.

I'm also not sure how lit.local.cfg interacts with the test prefix updater: currently, the only cases where we bulk-want to allow unused prefixes is the Attributor tests. If you were going to add the flag explicitly, that'd also work. Or just the option to the update_test_prefix that says "ok with duplicates, don't warn"

I'm not sure how this is more helpful. What is the use case where this way of warning helps?

For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)

Should not we check for that flag in the RUN line then and only warn for unused prefixes when it is set. If there is no prefix we should obviously always warn.

That's a good idea. Probably we'd need to also make sure that the unused prefixes are all on RUN lines with --allow-unused-prefixes=true.

I'm also not sure how lit.local.cfg interacts with the test prefix updater: currently, the only cases where we bulk-want to allow unused prefixes is the Attributor tests. If you were going to add the flag explicitly, that'd also work. Or just the option to the update_test_prefix that says "ok with duplicates, don't warn"

I can add the option explicitly (D94744). We should look for the filecheck one if possible, two options means double the hassle. That said, why are we warning in both FileCheck and update_test_check, it seems to be unnecessary to do the latter.

I'm not sure how this is more helpful. What is the use case where this way of warning helps?

For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)

Should not we check for that flag in the RUN line then and only warn for unused prefixes when it is set. If there is no prefix we should obviously always warn.

That's a good idea. Probably we'd need to also make sure that the unused prefixes are all on RUN lines with --allow-unused-prefixes=true.

I'm also not sure how lit.local.cfg interacts with the test prefix updater: currently, the only cases where we bulk-want to allow unused prefixes is the Attributor tests. If you were going to add the flag explicitly, that'd also work. Or just the option to the update_test_prefix that says "ok with duplicates, don't warn"

I can add the option explicitly (D94744). We should look for the filecheck one if possible, two options means double the hassle. That said, why are we warning in both FileCheck and update_test_check, it seems to be unnecessary to do the latter.

update_test_prefix.py's role is temporary: once we flip FileCheck to disallow unused prefixes by default, we don't need to keep it around. At that point, it becomes important for the update_<xyz>_test_check scripts to warn.

I'm not sure how this is more helpful. What is the use case where this way of warning helps?

For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)

Should not we check for that flag in the RUN line then and only warn for unused prefixes when it is set. If there is no prefix we should obviously always warn.

That's a good idea. Probably we'd need to also make sure that the unused prefixes are all on RUN lines with --allow-unused-prefixes=true.

I'm also not sure how lit.local.cfg interacts with the test prefix updater: currently, the only cases where we bulk-want to allow unused prefixes is the Attributor tests. If you were going to add the flag explicitly, that'd also work. Or just the option to the update_test_prefix that says "ok with duplicates, don't warn"

I can add the option explicitly (D94744). We should look for the filecheck one if possible, two options means double the hassle. That said, why are we warning in both FileCheck and update_test_check, it seems to be unnecessary to do the latter.

update_test_prefix.py's role is temporary: once we flip FileCheck to disallow unused prefixes by default, we don't need to keep it around. At that point, it becomes important for the update_<xyz>_test_check scripts to warn.

I don't follow. I would assume it's the opposite. If FileCheck doesn't allow unused prefixes why warn in the update script as well. Anyway, there should be a way to opt-out.

I'm not sure how this is more helpful. What is the use case where this way of warning helps?

For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)

Should not we check for that flag in the RUN line then and only warn for unused prefixes when it is set. If there is no prefix we should obviously always warn.

That's a good idea. Probably we'd need to also make sure that the unused prefixes are all on RUN lines with --allow-unused-prefixes=true.

I'm also not sure how lit.local.cfg interacts with the test prefix updater: currently, the only cases where we bulk-want to allow unused prefixes is the Attributor tests. If you were going to add the flag explicitly, that'd also work. Or just the option to the update_test_prefix that says "ok with duplicates, don't warn"

I can add the option explicitly (D94744). We should look for the filecheck one if possible, two options means double the hassle. That said, why are we warning in both FileCheck and update_test_check, it seems to be unnecessary to do the latter.

update_test_prefix.py's role is temporary: once we flip FileCheck to disallow unused prefixes by default, we don't need to keep it around. At that point, it becomes important for the update_<xyz>_test_check scripts to warn.

I don't follow. I would assume it's the opposite. If FileCheck doesn't allow unused prefixes why warn in the update script as well. Anyway, there should be a way to opt-out.

I'm not sure how this is more helpful. What is the use case where this way of warning helps?

For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)

Should not we check for that flag in the RUN line then and only warn for unused prefixes when it is set. If there is no prefix we should obviously always warn.

That's a good idea. Probably we'd need to also make sure that the unused prefixes are all on RUN lines with --allow-unused-prefixes=true.

I'm also not sure how lit.local.cfg interacts with the test prefix updater: currently, the only cases where we bulk-want to allow unused prefixes is the Attributor tests. If you were going to add the flag explicitly, that'd also work. Or just the option to the update_test_prefix that says "ok with duplicates, don't warn"

I can add the option explicitly (D94744). We should look for the filecheck one if possible, two options means double the hassle. That said, why are we warning in both FileCheck and update_test_check, it seems to be unnecessary to do the latter.

update_test_prefix.py's role is temporary: once we flip FileCheck to disallow unused prefixes by default, we don't need to keep it around. At that point, it becomes important for the update_<xyz>_test_check scripts to warn.

I don't follow. I would assume it's the opposite. If FileCheck doesn't allow unused prefixes why warn in the update script as well. Anyway, there should be a way to opt-out.

There's an earlier comment in this patch about that, anchored to line 296 of common.py. IIUC, a developer: 1) updates their test by adding new RUN lines, maybe adding prefixes to existing RUN lines, 2) runs the appropriate update_xyz_checks.py. Then, indeed, they could run the test and see FileCheck's warnings, but they might be confused. Instead, we can pre-warn. Eventually, we could imagine giving a bit more information in the warning as to which RUN line that was, for example.

I'll look at adding --allow-unused-prefixes=true awareness shortly.

mtrofin closed this revision.Jan 19 2021, 8:24 AM