[PATCH] Add CHECK-LABEL directive to FileCheck to allow more accurate error messages and error recovery
Eli Bendersky
eliben at google.com
Thu Jul 11 13:40:39 PDT 2013
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.
>
I explained my preference, and LABEL suggests something that is not
necessarily true - since it doesn't have to be attached to a label. Your
documentation and patch description speaks about "breaking to blocks",
which has the spatial semantics I was referring to earlier and no relation
to labels (which are just one example of unique identifiers that may serve
as block boundaries).
That said, there's only a certain amount of time I'm willing to spend on
such bikeshedding - so if I'm in minority w.r.t the name, so be it.
Eli
> 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130711/5ba03c13/attachment.html>
More information about the llvm-commits
mailing list