[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
Mon Feb 8 13:49:50 PST 2021 

https://reviews.llvm.org/D96290 should take care of this

On Fri, Feb 5, 2021 at 9:09 AM Mircea Trofin <mtrofin at google.com> wrote:

>
>
> 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/20210208/c1d3ecc2/attachment-0001.html>

More information about the llvm-dev mailing list