[llvm-dev] Conflicting check prefix detection not working in update_llc_test_checks.py?

Mircea Trofin via llvm-dev llvm-dev at lists.llvm.org
Fri Feb 5 09:09:51 PST 2021 

On Fri, Feb 5, 2021 at 8:50 AM Sanjay Patel <spatel at rotateright.com> wrote:

> Yes, warning only if a function has no asserts sounds like it would
> restore the previous behavior?
>
The previous *intended (probably)* behavior  :).

The previous behavior would have warned when 1) a function lost an assert
due to a conflict, and 2) that prefix was the last one in the RUN list that
happened to have caused point 1. So sometimes, depending on how RUN lines
were set up, it may have happened to be the behavior we described here.
Often it was just a silent failure, and/or it was resulting in an invalid
test.

I'll create a patch with the desired behavior shortly - thanks for
clarifying the problem space!


> If I modify the example test file with a common prefix like this, then I
> agree with Pengfei that there should be no warnings - all functions have
> the expected (either common or unique) assertions:
>
> diff --git a/llvm/test/CodeGen/RISCV/alu32.ll
> b/llvm/test/CodeGen/RISCV/alu32.ll
> index 975716e53c04..bf74ef5eab6f 100644
> --- a/llvm/test/CodeGen/RISCV/alu32.ll
> +++ b/llvm/test/CodeGen/RISCV/alu32.ll
> @@ -1,292 +1,252 @@
>  ; NOTE: Assertions have been autogenerated by
> utils/update_llc_test_checks.py
>  ; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
> -; RUN:   | FileCheck %s -check-prefix=RV32I
> +; RUN:   | FileCheck %s --check-prefixes=CHECK,RV32I
>  ; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
> -; RUN:   | FileCheck %s -check-prefix=RV64I
> +; RUN:   | FileCheck %s --check-prefixes=CHECK,RV64I
>
> On Fri, Feb 5, 2021 at 10:57 AM Mircea Trofin <mtrofin at google.com> wrote:
>
>> Thanks - so would it be then desirable to just warn *if* a function ends
>> up stripped of any assertions? I.e. say you have 2 functions, f1 and f2.
>> First RUN line produces an output for both for prefixes "P1 and P2". Second
>> RUN line produces a conflicting output for f1 for P1 (f2 is OK). Third RUN
>> line produces a conflicting output for f1 for P2. Result: f1 has no asserts
>> => we warn.
>>
>> On Thu, Feb 4, 2021 at 11:22 PM Wang, Pengfei <pengfei.wang at intel.com>
>> wrote:
>>
>>> It looks to me the "Function ..." warnings are unnecessary. The test has
>>> been updated as expected.
>>> I agreed with Craig. I was thinking of this pain point when I removed
>>> those used prefixes.
>>> I believe it's of little account for experienced users like Craig, Simon
>>> and Sanjay who adding finer grained prefixes for X86.
>>> But I'm worry about junior user or developers form other targets who
>>> accidently changes X86 tests.
>>>
>>> Thanks
>>> Pengfei
>>>
>>> -----Original Message-----
>>> From: Fāng-ruì Sòng <maskray at google.com>
>>> Sent: Friday, February 5, 2021 2:34 PM
>>> To: Craig Topper <craig.topper at gmail.com>
>>> Cc: Mircea Trofin <mtrofin at google.com>; llvm-dev <
>>> llvm-dev at lists.llvm.org>; Simon Pilgrim <llvm-dev at redking.me.uk>;
>>> Sanjay Patel <spatel at rotateright.com>; Wang, Pengfei <
>>> pengfei.wang at intel.com>
>>> Subject: Re: [llvm-dev] Conflicting check prefix detection not working
>>> in update_llc_test_checks.py?
>>>
>>> Add back dropped CC: Simon and Sanjay.
>>>
>>> They use update_llc_test_checks.py heavily, perhaps, they can help? :)
>>>
>>> On Wed, Feb 3, 2021 at 11:25 AM Craig Topper via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>> >
>>> > Maybe. I'm not quite sure what you mean by "functions with no asm".
>>> >
>>> > X86 especially has a lot of tests with a half dozen or more RUN lines.
>>> There is a common prefix for all RUN lines or at least all AVX/AVX512 run
>>> lines and then successively more precise check prefixes grouping smaller
>>> and smaller subsets of RUN lines. The goal is to minimize the number of
>>> check lines in the file by grouping as much as possible. But allowing
>>> precise checks for individual functions that have different behavior on
>>> many targets.
>>> >
>>> > Since we can't have unused prefixes in the test files any more we
>>> might lose some of the finer grained check prefixes if multiple RUN lines
>>> produce the same results for all functions today. A future change may break
>>> that and require finer grained prefixes to be added. That's the important
>>> case to warn about since it requires the user to change the RUN lines.
>>> >
>>> > ~Craig
>>> >
>>> >
>>> > On Tue, Feb 2, 2021 at 9:39 PM Mircea Trofin <mtrofin at google.com>
>>> wrote:
>>> >>
>>> >> I see, so maybe there are some levels of warning to observe -
>>> "everything", functions with no asm, prefixes with no use. And maybe the
>>> default is the second?
>>> >>
>>> >> On Tue, Feb 2, 2021, 21:35 Craig Topper <craig.topper at gmail.com>
>>> wrote:
>>> >>>
>>> >>> Hi Mircea,
>>> >>>
>>> >>> It looks like the script is now reporting warnings even when there
>>> is a set of prefixes to update the test for all functions. For example, if
>>> you add -check-prefixes=CHECK,RV32I to the first command in alu32.ll and
>>> -check-prefixes=CHECK,RV64I to the second you'll get this, but it looks
>>> like there's a valid solution for the test and no user intervention is
>>> required.
>>> >>>
>>> >>> WARNING: Function slti had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function sltiu had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function srli had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function srai had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function add had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function sub had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function sll had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function slt had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function sltu had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function srl had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function sra had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>>
>>> >>> ~Craig
>>> >>>
>>> >>>
>>> >>> On Mon, Feb 1, 2021 at 3:19 PM Craig Topper <craig.topper at gmail.com>
>>> wrote:
>>> >>>>
>>> >>>> Yes, that matches my expectations. Thanks!
>>> >>>>
>>> >>>> ~Craig
>>> >>>>
>>> >>>>
>>> >>>> On Mon, Feb 1, 2021 at 3:05 PM Mircea Trofin <mtrofin at google.com>
>>> wrote:
>>> >>>>>
>>> >>>>> Indeed, we're now not output-ing the case where some functions
>>> have conflicting asm, just the case when all functions lose their asm.
>>> >>>>>
>>> >>>>> I have a fix ready; to confirm, for this example (i.e. taking all
>>> (both) the "--check-prefix"-es in alu32.ll), would this output match your
>>> expectations?
>>> >>>>>
>>> >>>>> WARNING: Function slti had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function sltiu had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function srli had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function srai had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function add had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function sub had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function sll had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function slt had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function sltu had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function srl had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function sra had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>>
>>> >>>>> On Mon, Feb 1, 2021 at 2:12 PM Mircea Trofin <mtrofin at google.com>
>>> wrote:
>>> >>>>>>
>>> >>>>>> looking
>>> >>>>>>
>>> >>>>>> On Mon, Feb 1, 2021 at 2:11 PM Craig Topper <
>>> craig.topper at gmail.com> wrote:
>>> >>>>>>>
>>> >>>>>>> update_llc_test_checks.py seems to not be telling me about
>>> assembly that differs under the same prefix anymore.
>>> >>>>>>>
>>> >>>>>>> An easy way to see this is to just remove the --check-prefix
>>> from test/CodeGen/RISCV/alu32.ll and run the script. You'll get no error
>>> about conflicts. And if you look at the resulting file only some functions
>>> will have been updated to use CHECK as the prefix.
>>> >>>>>>>
>>> >>>>>>> Reverting some commits to update_llc_test_checks.py suggest this
>>> >>>>>>> may have been broken by e2dc306b1ac71258e6ce40a66e778527f282c355
>>> >>>>>>> [utils] Fix UpdateTestChecks case where 2 runs differ for last
>>> >>>>>>> label
>>> >>>>>>>
>>> >>>>>>> ~Craig
>>> >
>>> > _______________________________________________
>>> > LLVM Developers mailing list
>>> > llvm-dev at lists.llvm.org
>>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>>
>>>
>>> --
>>> 宋方睿
>>>
>>
> On Fri, Feb 5, 2021 at 10:57 AM Mircea Trofin <mtrofin at google.com> wrote:
>
>> Thanks - so would it be then desirable to just warn *if* a function ends
>> up stripped of any assertions? I.e. say you have 2 functions, f1 and f2.
>> First RUN line produces an output for both for prefixes "P1 and P2". Second
>> RUN line produces a conflicting output for f1 for P1 (f2 is OK). Third RUN
>> line produces a conflicting output for f1 for P2. Result: f1 has no asserts
>> => we warn.
>>
>> On Thu, Feb 4, 2021 at 11:22 PM Wang, Pengfei <pengfei.wang at intel.com>
>> wrote:
>>
>>> It looks to me the "Function ..." warnings are unnecessary. The test has
>>> been updated as expected.
>>> I agreed with Craig. I was thinking of this pain point when I removed
>>> those used prefixes.
>>> I believe it's of little account for experienced users like Craig, Simon
>>> and Sanjay who adding finer grained prefixes for X86.
>>> But I'm worry about junior user or developers form other targets who
>>> accidently changes X86 tests.
>>>
>>> Thanks
>>> Pengfei
>>>
>>> -----Original Message-----
>>> From: Fāng-ruì Sòng <maskray at google.com>
>>> Sent: Friday, February 5, 2021 2:34 PM
>>> To: Craig Topper <craig.topper at gmail.com>
>>> Cc: Mircea Trofin <mtrofin at google.com>; llvm-dev <
>>> llvm-dev at lists.llvm.org>; Simon Pilgrim <llvm-dev at redking.me.uk>;
>>> Sanjay Patel <spatel at rotateright.com>; Wang, Pengfei <
>>> pengfei.wang at intel.com>
>>> Subject: Re: [llvm-dev] Conflicting check prefix detection not working
>>> in update_llc_test_checks.py?
>>>
>>> Add back dropped CC: Simon and Sanjay.
>>>
>>> They use update_llc_test_checks.py heavily, perhaps, they can help? :)
>>>
>>> On Wed, Feb 3, 2021 at 11:25 AM Craig Topper via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>> >
>>> > Maybe. I'm not quite sure what you mean by "functions with no asm".
>>> >
>>> > X86 especially has a lot of tests with a half dozen or more RUN lines.
>>> There is a common prefix for all RUN lines or at least all AVX/AVX512 run
>>> lines and then successively more precise check prefixes grouping smaller
>>> and smaller subsets of RUN lines. The goal is to minimize the number of
>>> check lines in the file by grouping as much as possible. But allowing
>>> precise checks for individual functions that have different behavior on
>>> many targets.
>>> >
>>> > Since we can't have unused prefixes in the test files any more we
>>> might lose some of the finer grained check prefixes if multiple RUN lines
>>> produce the same results for all functions today. A future change may break
>>> that and require finer grained prefixes to be added. That's the important
>>> case to warn about since it requires the user to change the RUN lines.
>>> >
>>> > ~Craig
>>> >
>>> >
>>> > On Tue, Feb 2, 2021 at 9:39 PM Mircea Trofin <mtrofin at google.com>
>>> wrote:
>>> >>
>>> >> I see, so maybe there are some levels of warning to observe -
>>> "everything", functions with no asm, prefixes with no use. And maybe the
>>> default is the second?
>>> >>
>>> >> On Tue, Feb 2, 2021, 21:35 Craig Topper <craig.topper at gmail.com>
>>> wrote:
>>> >>>
>>> >>> Hi Mircea,
>>> >>>
>>> >>> It looks like the script is now reporting warnings even when there
>>> is a set of prefixes to update the test for all functions. For example, if
>>> you add -check-prefixes=CHECK,RV32I to the first command in alu32.ll and
>>> -check-prefixes=CHECK,RV64I to the second you'll get this, but it looks
>>> like there's a valid solution for the test and no user intervention is
>>> required.
>>> >>>
>>> >>> WARNING: Function slti had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function sltiu had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function srli had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function srai had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function add had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function sub had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function sll had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function slt had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function sltu had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function srl had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>> WARNING: Function sra had conflicting output from different RUN
>>> >>> lines for prefix CHECK
>>> >>>
>>> >>>
>>> >>> ~Craig
>>> >>>
>>> >>>
>>> >>> On Mon, Feb 1, 2021 at 3:19 PM Craig Topper <craig.topper at gmail.com>
>>> wrote:
>>> >>>>
>>> >>>> Yes, that matches my expectations. Thanks!
>>> >>>>
>>> >>>> ~Craig
>>> >>>>
>>> >>>>
>>> >>>> On Mon, Feb 1, 2021 at 3:05 PM Mircea Trofin <mtrofin at google.com>
>>> wrote:
>>> >>>>>
>>> >>>>> Indeed, we're now not output-ing the case where some functions
>>> have conflicting asm, just the case when all functions lose their asm.
>>> >>>>>
>>> >>>>> I have a fix ready; to confirm, for this example (i.e. taking all
>>> (both) the "--check-prefix"-es in alu32.ll), would this output match your
>>> expectations?
>>> >>>>>
>>> >>>>> WARNING: Function slti had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function sltiu had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function srli had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function srai had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function add had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function sub had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function sll had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function slt had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function sltu had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function srl had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>> WARNING: Function sra had conflicting output from different RUN
>>> >>>>> lines for prefix CHECK
>>> >>>>>
>>> >>>>> On Mon, Feb 1, 2021 at 2:12 PM Mircea Trofin <mtrofin at google.com>
>>> wrote:
>>> >>>>>>
>>> >>>>>> looking
>>> >>>>>>
>>> >>>>>> On Mon, Feb 1, 2021 at 2:11 PM Craig Topper <
>>> craig.topper at gmail.com> wrote:
>>> >>>>>>>
>>> >>>>>>> update_llc_test_checks.py seems to not be telling me about
>>> assembly that differs under the same prefix anymore.
>>> >>>>>>>
>>> >>>>>>> An easy way to see this is to just remove the --check-prefix
>>> from test/CodeGen/RISCV/alu32.ll and run the script. You'll get no error
>>> about conflicts. And if you look at the resulting file only some functions
>>> will have been updated to use CHECK as the prefix.
>>> >>>>>>>
>>> >>>>>>> Reverting some commits to update_llc_test_checks.py suggest this
>>> >>>>>>> may have been broken by e2dc306b1ac71258e6ce40a66e778527f282c355
>>> >>>>>>> [utils] Fix UpdateTestChecks case where 2 runs differ for last
>>> >>>>>>> label
>>> >>>>>>>
>>> >>>>>>> ~Craig
>>> >
>>> > _______________________________________________
>>> > LLVM Developers mailing list
>>> > llvm-dev at lists.llvm.org
>>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>>
>>>
>>> --
>>> 宋方睿
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210205/f3af739a/attachment-0001.html>

More information about the llvm-dev mailing list