[LLVMdev] Bikeshedding a name for new directive: CHECK-LABEL vs. CHECK-BOUNDARY vs. something else.
Stephen Lin
swlin at post.harvard.edu
Thu Jul 11 13:41:11 PDT 2013
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