[LLVMdev] Bikeshedding a name for new directive: CHECK-LABEL vs. CHECK-BOUNDARY vs. something else.
Stephen Lin
swlin at post.harvard.edu
Fri Jul 12 07:53:24 PDT 2013
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
>>>
>>>
More information about the llvm-dev
mailing list