Page MenuHomePhabricator

[tools] UpdateTestPrefix: improved verbosity
Needs ReviewPublic

Authored by mtrofin on Feb 8 2021, 1:49 PM.

Details

Summary

Warning by default only if a function ends up not having any matching
output for any prefixes - as well as unused prefixes (which should be
none/minimal noise)

Diff Detail

Unit TestsFailed

TimeTest
80 msx64 debian > Polly.ScopInfo::user_provided_assumptions.ll
Script: -- : 'RUN: at line 1'; opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/mnt/disks/ssd0/agent/llvm-project/polly/test/ScopInfo -polly-codegen-verify -pass-remarks-analysis="polly-scops" -polly-scops -disable-output < /mnt/disks/ssd0/agent/llvm-project/polly/test/ScopInfo/user_provided_assumptions.ll 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/polly/test/ScopInfo/user_provided_assumptions.ll
380 msx64 debian > libarcher.races::task-dependency.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-dependency.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-dependency.c
370 msx64 debian > libarcher.races::task-taskgroup-unrelated.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskgroup-unrelated.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskgroup-unrelated.c
390 msx64 debian > libarcher.races::task-taskwait-nested.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskwait-nested.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskwait-nested.c
350 msx64 debian > libarcher.races::task-two.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-two.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-two.c
View Full Test Results (14 Failed)

Event Timeline

mtrofin created this revision.Feb 8 2021, 1:49 PM
mtrofin requested review of this revision.Feb 8 2021, 1:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 1:49 PM

I've added a functionality question, but someone else should definitely look at the implementation because my python experience is minimal.

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/prefix_warning_verbosity_nowarn.ll
5

If we remove "RV64I" here, then the last 2 functions would have no assertions, right? Do we want to issue a warning for that scenario? Right now, it seems to be silent on that case by default.

mtrofin marked an inline comment as done.Feb 9 2021, 2:55 PM
mtrofin added inline comments.
llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/prefix_warning_verbosity_nowarn.ll
5

No, the last 2 functions will have the RV32I assertions from the RUN line above.

spatel added inline comments.Feb 11 2021, 9:15 AM
llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/prefix_warning_verbosity_nowarn.ll
5

Ah, I see. But those functions would have no assertions for the 2nd RUN line. Is there a way to enable warnings for that case?

mtrofin marked 2 inline comments as done.Feb 11 2021, 9:50 AM
mtrofin added inline comments.
llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/prefix_warning_verbosity_nowarn.ll
5

That'd be outside the scope of this fix - this one just aims to provide the expected behavior that was sometimes working (because it was buggy) pre-e2dc306b1ac71258e6ce40a66e778527f282c355.

mtrofin marked an inline comment as done.Feb 15 2021, 8:36 AM

gentle reminder

I think this is an improvement from current behavior, so no objections from me, but hopefully someone else can take a look at the implementation.
@RKSimon might have some comments based on:
https://llvm.org/PR49189

I think this is an improvement from current behavior, so no objections from me, but hopefully someone else can take a look at the implementation.
@RKSimon might have some comments based on:
https://llvm.org/PR49189

I've replied on the bug , but we seem to have lost the original warning as part of D93078

I think this is an improvement from current behavior, so no objections from me, but hopefully someone else can take a look at the implementation.
@RKSimon might have some comments based on:
https://llvm.org/PR49189

I've replied on the bug , but we seem to have lost the original warning as part of D93078

As detailed on the bug, and on D93078, that warning was buggy. In particular, in the case of PR49189, it wouldn't have worked (tried by sync-ing before the patch and running the tool).

Are we confident this (i.e. warn if a RUN line's outputs aren't used) was the intended behavior? Perhaps it would be worth taking a step back and defining what the desired behavior of the tool is - independent from previous state of the code which, again, was buggy and untested (and, thus, can't serve as a specification).

It comes down to us wanting the update scripts to use lists of check-prefixes for every RUN line, allowing us to share prefixes at the function level, typically getting more and more specific down the list. That way we can minimise bloat in the check lines, but still have thorough coverage with many RUNs.

With the move to avoid unused prefixes, there's going to be a lot more cases where the prefix lists need editing every time a test is changed - we need the update scripts to tell us both when a prefix becomes newly unused, but also when a run suddenly isn't checked at all.

It comes down to us wanting the update scripts to use lists of check-prefixes for every RUN line, allowing us to share prefixes at the function level, typically getting more and more specific down the list. That way we can minimise bloat in the check lines, but still have thorough coverage with many RUNs.

With the move to avoid unused prefixes, there's going to be a lot more cases where the prefix lists need editing every time a test is changed - we need the update scripts to tell us both when a prefix becomes newly unused, but also when a run suddenly isn't checked at all.

I'm confused - IIRC (https://lists.llvm.org/pipermail/llvm-dev/2021-February/148309.html), the scenario for update_[llc]_test_checks was better nuanced, and it seemed like the position was that a) those tests could, indeed, validly feature unused prefixes, and b) the tools (update_[]_test_checks) would potentially handle that, or the tests would explicitly opt in.

This patch was about addressing overly-verbose messages - https://lists.llvm.org/pipermail/llvm-dev/2021-February/148272.html