thr3ads.net - llvm dev - [LLVMdev] Bikeshedding a name for new directive: CHECK-LABEL vs. CHECK-BOUNDARY vs. something else. [Jul 2013]

If this information is useful, please help other people find it:
Share via:

Stephen Lin

2013-Jul-11 20:41 UTC

[LLVMdev] Bikeshedding a name for new directive: CHECK-LABEL vs. CHECK-BOUNDARY vs. something else.

Hi,

I would like to add a new directive to FileCheck called CHECK-FOO
(where FOO is a name under discussion right now) which is used to
improve error messages. The idea is that you would use CHECK-FOO on
any line that contains a unique identifier (typically labels, function
definitions, etc.) that is guaranteed to only occur once in the file;
FileCheck will then conceptually break the break the input into blocks
separated by these unique identifier lines and perform all other
checks localized to between the appropriate blocks; it can ever
recover from an error in one block and move on to another.

As an example, I purposely introduced the a switch fall-through bug in
the last patch I submitted to llvm-commits ("Allow FMAs in safe math
mode in some cases when one operand of the fmul is either exactly 0.0
or exactly 1.0")...

Bug diff:

diff --git a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 0290afc..239b119 100644
--- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -5791,7 +5791,7 @@ static bool isExactlyZeroOrOne(const
TargetLowering &TLI, const SDValue &Op) {
           continue;
         }
       }
-      break;
+//      break;
     case ISD::FADD:
       if (ConstantFPSDNode *V0CFP             
dyn_cast<ConstantFPSDNode>(V->getOperand(0))) {

The single error message without CHECK-FOO is:

; CHECK: test_add_8
         ^
<stdin>:125:2: note: scanning from here
 .cfi_endproc
 ^
<stdin>:127:10: note: possible intended match here
 .globl _test_add_10
         ^

The error messages with CHECK-FOO on the function name label lines are:

; CHECK: vmulsd
         ^
<stdin>:87:2: note: scanning from here
 .align 4, 0x90
 ^
<stdin>:95:2: note: possible intended match here
 vsubsd %xmm0, %xmm3, %xmm0
 ^
fp-contract.ll:118:15: error: expected string not found in input
; CHECK: vmulsd
         ^
<stdin>:102:2: note: scanning from here
 .align 4, 0x90
 ^
<stdin>:109:2: note: possible intended match here
 vsubsd %xmm2, %xmm3, %xmm2
 ^
fp-contract.ll:288:15: error: expected string not found in input
; CHECK: vmulsd
         ^
<stdin>:258:2: note: scanning from here
 .align 4, 0x90
 ^
<stdin>:266:2: note: possible intended match here
 vsubsd %xmm0, %xmm3, %xmm0
 ^

Does anyone have a suggestions on what FOO should be? In my current
patch it's currently LABEL, but Eli. B. suggested BOUNDARY

Any opinions or other suggestions?

Thanks,
Stephen

On Thu, Jul 11, 2013 at 1:33 PM, Stephen Lin <swlin at post.harvard.edu>
wrote:> It's just short for BOUNDARY. I think BOUNDARY is too long :D
> I prefer LABEL though. I can send this to the dev list and ask for
> opinions there.
> Stephen
>
> On Thu, Jul 11, 2013 at 12:54 PM, Eli Bendersky <eliben at
google.com> wrote:
>>
>> On Thu, Jul 11, 2013 at 12:44 PM, Stephen Lin <swlin at
post.harvard.edu>
>> wrote:
>>>
>>> Actually, I would be ok with CHECK-BOUND as well.
>>> Eli, is that OK to you? And does anyone else want to chime in?
>>> I will expand the docs either way.
>>> Thanks,
>>> Stephen
>>
>>
>> I'm not sure what BOUND means in this case? And how is it different
from
>> BOUNDARY?
>>
>> I'm just thinking of someone reading the test file and looking at
all the
>> directives. BOUNDARY conveys a spatial meaning and it's easy to
intuitively
>> remember what its semantics are. My opposition to LABEL was because
LABEL
>> conveyed no such meaning and I think it would be confusing. As for
BOUND vs.
>> BOUNDARY, that's really a minor issue and perhaps my knowledge of
English
>> fails me here, but I'd be happy to hear the reasoning.
>>
>> Eli
>>
>>
>>
>>
>>
>>>
>>>
>>> On Thu, Jul 11, 2013 at 12:40 PM, Stephen Lin <swlin at
post.harvard.edu>
>>> wrote:
>>> > Thanks Owen; Andy (Trick) off-list says he thinks it's a
good idea, too.
>>> >
>>> > Eli B. (also off-list) thinks that the documentation can be
approved
>>> > and also suggests that the name CHECK-BOUNDARY is better.
Anyone else
>>> > have an opinion?
>>> >
>>> > I much prefer CHECK-LABEL to CHECK-BOUNDARY myself, but I am
willing
>>> > to paint the bike shed whatever color others can agree on.
>>> >
>>> > Stephen
>>> >
>>> > On Thu, Jul 11, 2013 at 12:31 PM, Owen Anderson <resistor
at mac.com>
>>> > wrote:
>>> >> I'm not familiar enough with the FileCheck internals
to comment on the
>>> >> implementation, but I *really* like this feature. 
I've spent way too much
>>> >> time over the years tracking down cryptic FileCheck errors
that would have
>>> >> been solved by this.
>>> >>
>>> >> --Owen
>>> >>
>>> >> On Jul 11, 2013, at 10:50 AM, Stephen Lin <swlin at
post.harvard.edu>
>>> >> wrote:
>>> >>
>>> >>> Hi,
>>> >>>
>>> >>> Can anyone review this patch? It adds a new directive
type called
>>> >>> "CHECK-LABEL" to FileCheck...
>>> >>>
>>> >>> If present in a match file, FileCheck will use these
directives to
>>> >>> split the input into blocks that are independently
processed, ensuring
>>> >>> that a CHECK does not inadvertently match a line in a
different block
>>> >>> (which can lead to a misleading/useless error message
when the error
>>> >>> is eventually caught). Also, FileCheck can now recover
from errors
>>> >>> within blocks by continuing to the next block.
>>> >>>
>>> >>> As an example, I purposely introduced the a switch
fall-through bug in
>>> >>> the last patch I submitted to llvm-commits
("Allow FMAs in safe math
>>> >>> mode in some cases when one operand of the fmul is
either exactly 0.0
>>> >>> or exactly 1.0")...
>>> >>>
>>> >>> Bug diff:
>>> >>>
>>> >>> diff --git a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>> >>> b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>> >>> index 0290afc..239b119 100644
>>> >>> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>> >>> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>> >>> @@ -5791,7 +5791,7 @@ static bool
isExactlyZeroOrOne(const
>>> >>> TargetLowering &TLI, const SDValue &Op) {
>>> >>>           continue;
>>> >>>         }
>>> >>>       }
>>> >>> -      break;
>>> >>> +//      break;
>>> >>>     case ISD::FADD:
>>> >>>       if (ConstantFPSDNode *V0CFP >>>
>>>             dyn_cast<ConstantFPSDNode>(V->getOperand(0)))
{
>>> >>>
>>> >>> The single error message without CHECK-LABEL is:
>>> >>>
>>> >>> ; CHECK-SAFE: test_add_8
>>> >>>              ^
>>> >>> <stdin>:125:2: note: scanning from here
>>> >>> .cfi_endproc
>>> >>> ^
>>> >>> <stdin>:127:10: note: possible intended match
here
>>> >>> .globl _test_add_10
>>> >>>         ^
>>> >>>
>>> >>> The error messages with CHECK-LABEL are:
>>> >>>
>>> >>> ; CHECK-SAFE: vmulsd
>>> >>>              ^
>>> >>> <stdin>:87:2: note: scanning from here
>>> >>> .align 4, 0x90
>>> >>> ^
>>> >>> <stdin>:95:2: note: possible intended match here
>>> >>> vsubsd %xmm0, %xmm3, %xmm0
>>> >>> ^
>>> >>> fp-contract.ll:118:15: error: expected string not
found in input
>>> >>> ; CHECK-SAFE: vmulsd
>>> >>>              ^
>>> >>> <stdin>:102:2: note: scanning from here
>>> >>> .align 4, 0x90
>>> >>> ^
>>> >>> <stdin>:109:2: note: possible intended match
here
>>> >>> vsubsd %xmm2, %xmm3, %xmm2
>>> >>> ^
>>> >>> fp-contract.ll:288:15: error: expected string not
found in input
>>> >>> ; CHECK-SAFE: vmulsd
>>> >>>              ^
>>> >>> <stdin>:258:2: note: scanning from here
>>> >>> .align 4, 0x90
>>> >>> ^
>>> >>> <stdin>:266:2: note: possible intended match
here
>>> >>> vsubsd %xmm0, %xmm3, %xmm0
>>> >>> ^
>>> >>>
>>> >>> The three error messages in the CHECK-LABEL case
exactly pinpoint the
>>> >>> source lines of the actual problem in three separate
blocks; the
>>> >>> single error message given without CHECK-LABEL is
(imho) much less
>>> >>> useful.
>>> >>>
>>> >>> (In this case, the non-CHECK-LABEL version happens to
error on the on
>>> >>> a label line, so the user could presume that the error
happened in the
>>> >>> block immediately before test_add_8, which is correct,
but in general
>>> >>> this might not be true; the only thing that can be
concluded is that
>>> >>> the error happened sometime before test_add_8.)
>>> >>>
>>> >>> Please let me know if you have any feedback.
>>> >>>
>>> >>> Stephen
>>> >>>
>>> >>> ---------- Forwarded message ----------
>>> >>> From: Stephen Lin <swlin at apple.com>
>>> >>> Date: Mon, Jun 10, 2013 at 4:21 PM
>>> >>> Subject: [PATCH] Add CHECK-LABEL directive to
FileCheck to allow more
>>> >>> accurate error messages and error recovery
>>> >>> To: llvm-commits at cs.uiuc.edu
>>> >>>
>>> >>>
>>> >>> Actually, I went ahead and renamed it CHECK-LABEL and
rebased, since I
>>> >>> think it’s better :)
>>> >>> -Stephen
>>> >>>
<check-label.patch>_______________________________________________
>>> >>> llvm-commits mailing list
>>> >>> llvm-commits at cs.uiuc.edu
>>> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> >>
>>> >>
>>> >> _______________________________________________
>>> >> llvm-commits mailing list
>>> >> llvm-commits at cs.uiuc.edu
>>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>

Stephen Lin

2013-Jul-12 14:53 UTC

head link

[LLVMdev] Bikeshedding a name for new directive: CHECK-LABEL vs. CHECK-BOUNDARY vs. something else.

OK, it was two votes to one so I went with CHECK-LABEL, r186162.

On Thu, Jul 11, 2013 at 1:41 PM, Stephen Lin <swlin at post.harvard.edu>
wrote:> Hi,
>
> I would like to add a new directive to FileCheck called CHECK-FOO
> (where FOO is a name under discussion right now) which is used to
> improve error messages. The idea is that you would use CHECK-FOO on
> any line that contains a unique identifier (typically labels, function
> definitions, etc.) that is guaranteed to only occur once in the file;
> FileCheck will then conceptually break the break the input into blocks
> separated by these unique identifier lines and perform all other
> checks localized to between the appropriate blocks; it can ever
> recover from an error in one block and move on to another.
>
> As an example, I purposely introduced the a switch fall-through bug in
> the last patch I submitted to llvm-commits ("Allow FMAs in safe math
> mode in some cases when one operand of the fmul is either exactly 0.0
> or exactly 1.0")...
>
> Bug diff:
>
> diff --git a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> index 0290afc..239b119 100644
> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> @@ -5791,7 +5791,7 @@ static bool isExactlyZeroOrOne(const
> TargetLowering &TLI, const SDValue &Op) {
>            continue;
>          }
>        }
> -      break;
> +//      break;
>      case ISD::FADD:
>        if (ConstantFPSDNode *V0CFP >             
dyn_cast<ConstantFPSDNode>(V->getOperand(0))) {
>
> The single error message without CHECK-FOO is:
>
> ; CHECK: test_add_8
>          ^
> <stdin>:125:2: note: scanning from here
>  .cfi_endproc
>  ^
> <stdin>:127:10: note: possible intended match here
>  .globl _test_add_10
>          ^
>
> The error messages with CHECK-FOO on the function name label lines are:
>
> ; CHECK: vmulsd
>          ^
> <stdin>:87:2: note: scanning from here
>  .align 4, 0x90
>  ^
> <stdin>:95:2: note: possible intended match here
>  vsubsd %xmm0, %xmm3, %xmm0
>  ^
> fp-contract.ll:118:15: error: expected string not found in input
> ; CHECK: vmulsd
>          ^
> <stdin>:102:2: note: scanning from here
>  .align 4, 0x90
>  ^
> <stdin>:109:2: note: possible intended match here
>  vsubsd %xmm2, %xmm3, %xmm2
>  ^
> fp-contract.ll:288:15: error: expected string not found in input
> ; CHECK: vmulsd
>          ^
> <stdin>:258:2: note: scanning from here
>  .align 4, 0x90
>  ^
> <stdin>:266:2: note: possible intended match here
>  vsubsd %xmm0, %xmm3, %xmm0
>  ^
>
> Does anyone have a suggestions on what FOO should be? In my current
> patch it's currently LABEL, but Eli. B. suggested BOUNDARY
>
> Any opinions or other suggestions?
>
> Thanks,
> Stephen
>
> On Thu, Jul 11, 2013 at 1:33 PM, Stephen Lin <swlin at
post.harvard.edu> wrote:
>> It's just short for BOUNDARY. I think BOUNDARY is too long :D
>> I prefer LABEL though. I can send this to the dev list and ask for
>> opinions there.
>> Stephen
>>
>> On Thu, Jul 11, 2013 at 12:54 PM, Eli Bendersky <eliben at
google.com> wrote:
>>>
>>> On Thu, Jul 11, 2013 at 12:44 PM, Stephen Lin <swlin at
post.harvard.edu>
>>> wrote:
>>>>
>>>> Actually, I would be ok with CHECK-BOUND as well.
>>>> Eli, is that OK to you? And does anyone else want to chime in?
>>>> I will expand the docs either way.
>>>> Thanks,
>>>> Stephen
>>>
>>>
>>> I'm not sure what BOUND means in this case? And how is it
different from
>>> BOUNDARY?
>>>
>>> I'm just thinking of someone reading the test file and looking
at all the
>>> directives. BOUNDARY conveys a spatial meaning and it's easy to
intuitively
>>> remember what its semantics are. My opposition to LABEL was because
LABEL
>>> conveyed no such meaning and I think it would be confusing. As for
BOUND vs.
>>> BOUNDARY, that's really a minor issue and perhaps my knowledge
of English
>>> fails me here, but I'd be happy to hear the reasoning.
>>>
>>> Eli
>>>
>>>
>>>
>>>
>>>
>>>>
>>>>
>>>> On Thu, Jul 11, 2013 at 12:40 PM, Stephen Lin <swlin at
post.harvard.edu>
>>>> wrote:
>>>> > Thanks Owen; Andy (Trick) off-list says he thinks it's
a good idea, too.
>>>> >
>>>> > Eli B. (also off-list) thinks that the documentation can
be approved
>>>> > and also suggests that the name CHECK-BOUNDARY is better.
Anyone else
>>>> > have an opinion?
>>>> >
>>>> > I much prefer CHECK-LABEL to CHECK-BOUNDARY myself, but I
am willing
>>>> > to paint the bike shed whatever color others can agree on.
>>>> >
>>>> > Stephen
>>>> >
>>>> > On Thu, Jul 11, 2013 at 12:31 PM, Owen Anderson
<resistor at mac.com>
>>>> > wrote:
>>>> >> I'm not familiar enough with the FileCheck
internals to comment on the
>>>> >> implementation, but I *really* like this feature. 
I've spent way too much
>>>> >> time over the years tracking down cryptic FileCheck
errors that would have
>>>> >> been solved by this.
>>>> >>
>>>> >> --Owen
>>>> >>
>>>> >> On Jul 11, 2013, at 10:50 AM, Stephen Lin <swlin at
post.harvard.edu>
>>>> >> wrote:
>>>> >>
>>>> >>> Hi,
>>>> >>>
>>>> >>> Can anyone review this patch? It adds a new
directive type called
>>>> >>> "CHECK-LABEL" to FileCheck...
>>>> >>>
>>>> >>> If present in a match file, FileCheck will use
these directives to
>>>> >>> split the input into blocks that are independently
processed, ensuring
>>>> >>> that a CHECK does not inadvertently match a line
in a different block
>>>> >>> (which can lead to a misleading/useless error
message when the error
>>>> >>> is eventually caught). Also, FileCheck can now
recover from errors
>>>> >>> within blocks by continuing to the next block.
>>>> >>>
>>>> >>> As an example, I purposely introduced the a switch
fall-through bug in
>>>> >>> the last patch I submitted to llvm-commits
("Allow FMAs in safe math
>>>> >>> mode in some cases when one operand of the fmul is
either exactly 0.0
>>>> >>> or exactly 1.0")...
>>>> >>>
>>>> >>> Bug diff:
>>>> >>>
>>>> >>> diff --git
a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>> >>> b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>> >>> index 0290afc..239b119 100644
>>>> >>> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>> >>> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>> >>> @@ -5791,7 +5791,7 @@ static bool
isExactlyZeroOrOne(const
>>>> >>> TargetLowering &TLI, const SDValue &Op) {
>>>> >>>           continue;
>>>> >>>         }
>>>> >>>       }
>>>> >>> -      break;
>>>> >>> +//      break;
>>>> >>>     case ISD::FADD:
>>>> >>>       if (ConstantFPSDNode *V0CFP >>>>
>>>             dyn_cast<ConstantFPSDNode>(V->getOperand(0)))
{
>>>> >>>
>>>> >>> The single error message without CHECK-LABEL is:
>>>> >>>
>>>> >>> ; CHECK-SAFE: test_add_8
>>>> >>>              ^
>>>> >>> <stdin>:125:2: note: scanning from here
>>>> >>> .cfi_endproc
>>>> >>> ^
>>>> >>> <stdin>:127:10: note: possible intended
match here
>>>> >>> .globl _test_add_10
>>>> >>>         ^
>>>> >>>
>>>> >>> The error messages with CHECK-LABEL are:
>>>> >>>
>>>> >>> ; CHECK-SAFE: vmulsd
>>>> >>>              ^
>>>> >>> <stdin>:87:2: note: scanning from here
>>>> >>> .align 4, 0x90
>>>> >>> ^
>>>> >>> <stdin>:95:2: note: possible intended match
here
>>>> >>> vsubsd %xmm0, %xmm3, %xmm0
>>>> >>> ^
>>>> >>> fp-contract.ll:118:15: error: expected string not
found in input
>>>> >>> ; CHECK-SAFE: vmulsd
>>>> >>>              ^
>>>> >>> <stdin>:102:2: note: scanning from here
>>>> >>> .align 4, 0x90
>>>> >>> ^
>>>> >>> <stdin>:109:2: note: possible intended match
here
>>>> >>> vsubsd %xmm2, %xmm3, %xmm2
>>>> >>> ^
>>>> >>> fp-contract.ll:288:15: error: expected string not
found in input
>>>> >>> ; CHECK-SAFE: vmulsd
>>>> >>>              ^
>>>> >>> <stdin>:258:2: note: scanning from here
>>>> >>> .align 4, 0x90
>>>> >>> ^
>>>> >>> <stdin>:266:2: note: possible intended match
here
>>>> >>> vsubsd %xmm0, %xmm3, %xmm0
>>>> >>> ^
>>>> >>>
>>>> >>> The three error messages in the CHECK-LABEL case
exactly pinpoint the
>>>> >>> source lines of the actual problem in three
separate blocks; the
>>>> >>> single error message given without CHECK-LABEL is
(imho) much less
>>>> >>> useful.
>>>> >>>
>>>> >>> (In this case, the non-CHECK-LABEL version happens
to error on the on
>>>> >>> a label line, so the user could presume that the
error happened in the
>>>> >>> block immediately before test_add_8, which is
correct, but in general
>>>> >>> this might not be true; the only thing that can be
concluded is that
>>>> >>> the error happened sometime before test_add_8.)
>>>> >>>
>>>> >>> Please let me know if you have any feedback.
>>>> >>>
>>>> >>> Stephen
>>>> >>>
>>>> >>> ---------- Forwarded message ----------
>>>> >>> From: Stephen Lin <swlin at apple.com>
>>>> >>> Date: Mon, Jun 10, 2013 at 4:21 PM
>>>> >>> Subject: [PATCH] Add CHECK-LABEL directive to
FileCheck to allow more
>>>> >>> accurate error messages and error recovery
>>>> >>> To: llvm-commits at cs.uiuc.edu
>>>> >>>
>>>> >>>
>>>> >>> Actually, I went ahead and renamed it CHECK-LABEL
and rebased, since I
>>>> >>> think it’s better :)
>>>> >>> -Stephen
>>>> >>>
<check-label.patch>_______________________________________________
>>>> >>> llvm-commits mailing list
>>>> >>> llvm-commits at cs.uiuc.edu
>>>> >>>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> >>
>>>> >>
>>>> >> _______________________________________________
>>>> >> llvm-commits mailing list
>>>> >> llvm-commits at cs.uiuc.edu
>>>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>

Reasonably Related Threads

Search for more reasonably related threads

llvm dev - Jul 2013 - [LLVMdev] Bikeshedding a name for new directive: CHECK-LABEL vs. CHECK-BOUNDARY vs. something else.

[LLVMdev] Bikeshedding a name for new directive: CHECK-LABEL vs. CHECK-BOUNDARY vs. something else.

[LLVMdev] Bikeshedding a name for new directive: CHECK-LABEL vs. CHECK-BOUNDARY vs. something else.

Reasonably Related Threads