In D58784#1424297, @jhenderson wrote:

I think CHECK-FIRSTLINE: {{^$}} would be a natural consequence of not wanting to change the permitted uses of other check directives, although I definitely agree it's not ideal. I'm not sure how else it would work if CHECK-SAME remained illegal as the first CHECK, and if we don't want to change CHECK-EMPTY's semantics. I suppose we could special-case CHECK-EMPTY's behaviour when it is the first check in the file to match the first line. This would certainly be intuitive, but it means that CHECK-EMPTY would no longer mean the same as CHECK-NEXT (except matching an empty line) in all cases. Thus CHECK-EMPTY becomes "Checks if the NEXT line in the input is completely empty. If it is the first specified CHECK, it checks that the first line matches is empty." I think this would be nicer than a new directive for this specific case, or to force regular expressions etc.

In D58784#1419973, @probinson wrote:

This all lets me keep thinking that a CHECK-SAME as the first directive would naturally look for a match on the first line of input (i.e., the "same" line as the implicit initial label), and an initial CHECK-NEXT would (by virtue of how it is defined relative to the model, not because it's inherently useful) match on the second line of input.

If we are talking about matching a specific line (in this case the first line), I wonder if we could not extend the syntax of directives to accept a line number, e.g. CHECK-EMPTY-L1. Or if we think only the first line deserve to be checked explicitely simply a CHECK-FIRST. The fact that we are arguing about what would the meaning of CHECK-SAME and CHECK-NEXT on the first line be suggests that it would confuse lots of users. After all you are the most familiar people with that piece of software.

In D58784#1413770, @jhenderson wrote:

In D58784#1413769, @thopre wrote:

I think the asserts should stay, they ensure that the call to CountNumNewlinesBetween is counting the newline in the right segment of text.

I'm not sure I follow how the asserts ensure this? All it shows is that the current buffer pointer is not the same as the start of the memory block it is in, i.e. that it has moved forwards.

In any case I do not understand why the asserts search from the start of the buffer given that there is a CHECK before the CHECK-SAME and CHECK-NEXT in your examples. Could you detail a bit?

The Buffer contains the remainder of the text in the input following the previous match, or the entire text if it is the first match. For example, if the input string was abc and there was a CHECK: a first, Buffer contains bc, wheras CHECK: b would result in the Buffer just containing c. The problem is that the pattern CHECK: {{^$}} matches exactly 0 characters, thus the Buffer is the same before and after the match, and therefore the Buffer is still pointing at the start of the memory block.

I think the asserts should stay, they ensure that the call to CountNumNewlinesBetween is counting the newline in the right segment of text. I'd rather add an extra condition to the assert "or it's first NEXT". In any case I do not understand why the asserts search from the start of the buffer given that there is a CHECK before the CHECK-SAME and CHECK-NEXT in your examples. Could you detail a bit?

Abandon it as issue has already been fixed.

Someone beat me to it overnight and went for my initial approach. Is it worth a separate commit to stop testing for FileCheck's name in the error string as suggested or should I abandon this revision?

Remove program name from the string being checked for

No longer working for Arm. This would need to be rebased first if anybody picks it up.

No longer working for Arm. This would need to be rebased first if anybody picks it up.

As suggested by Joel, I'll start a discussion on llvm-dev to solve the real issue.

LGTM. Thanks

Forgot some nits about the description (please ignore if it doesn't end up in the commit message):

In D56789#1371929, @jdenny wrote:

In D56789#1371821, @thopre wrote:

[SNIP]

Agreed, something like CHECK-DAG-NEXT would be better suited. The requirement of CHECK-DAG with CHECK-NEXT was mentioned to me by a coworker and I noticed the weird inconsistency regarding whether CHECK-DAG is the first check directives or not and thought I'd fix it. The behaviour we need is indeed reordering without any line not checked for in between using CHECK-DAG is not the right solution. So I guess the correct approach would be to error out on CHECK-DAG with CHECK-NEXT, CHECK-SAME or CHECK-EMPTY, probably with some flag to enable the old behavior while tests are updated.

Possibly so. Given that many of those existing cases were probably intending the stricter behavior, let's first create a better solution for the stricter behavior.

Regarding CHECK-DAG-NEXT, I wonder how it would interact with CHECK-DAG?

I think that any kind of CHECK-DAG* following a different kind of CHECK-DAG* would start a new block such that there's no reordering among blocks. Otherwise, the semantics become very unclear to me.

It seems intuitive that the first match in a CHECK-DAG-NEXT block would have to appear on the line after whatever match came before the block. However, if you don't want that behavior, then I'm not sure how you would express that.

Splendid, thanks for addressing all my remarks and thanks ruiu for the excellent output formatting suggestion. LGTM.

FYI: only reviewed the SelectionDAGBuilder parts.

Besides typo and wording improvements:

Fix typos and improve title

In D56789#1368287, @jdenny wrote:

Hi Thomas,

Thanks for working to clean up this area.

As I understand it, CHECK-NEXT, CHECK-SAME, and CHECK-EMPTY currently can appear after non-initial CHECK-DAG blocks, and this patch intends to permit them after initial CHECK-DAG blocks as well. I agree with Paul that the title should be adjusted to reflect that.

In D57030#1369239, @jhenderson wrote:

In D57030#1367916, @ruiu wrote:

I found this file: llvm/unittests/Support/CommandLineTest.cpp

Maybe you can add a test for the new functionality to that file.

That's where I would test this, yes, but the function is all about printing to outs(), and I don't know of a way using LLVM's tools to test that. Can you (or anybody else) suggest anything?

I guess I could just put a test in "test/Other" which runs a tools with --help that prints the various forms available (although I don't think the "=<empty>" path is used anywhere currently).

In D57030#1367857, @jhenderson wrote:

In D57030#1367818, @thopre wrote:

I like this new version very much. I agree, <empty> is less likely to be confusing. One last nit: why do you check for the description being empty when deciding to skip an empty value for a ValueOptional option in the second block but not in the first block? Any reason for the inconsistency?

It wasn't intentional, but I think it is okay (although I could be persuaded to do something different). As things stand, an empty string with help text would produce something like this for a ValueOptional option:

-mhvx                                            - Enable Hexagon Vector eXtensions
 -mhvx=<value>                                    - Enable Hexagon Vector eXtensions
   =v60                                           -   Build for HVX v60
   =v62                                           -   Build for HVX v62
   =v65                                           -   Build for HVX v65
   =v66                                           -   Build for HVX v66
   =<empty>                                       -   Same as v66

We could instead suppress the =<empty> line always (even if it has a description), or we could always print the =<empty> line regardless of whether it has a description or not, I don't mind.

Ping?

I like this new version very much. I agree, <empty> is less likely to be confusing. One last nit: why do you check for the description being empty when deciding to skip an empty value for a ValueOptional option in the second block but not in the first block? Any reason for the inconsistency?

In D57030#1367059, @ruiu wrote:

Ideally I believe we should print out something like this for an option that takes an optional argument:

-mhvx                                            - Enable Hexagon Vector eXtensions
-mhvx=<value>                                    - Enable Hexagon Vector eXtensions
   =v60                                           -   Build for HVX v60
   =v62                                           -   Build for HVX v62
   =v65                                           -   Build for HVX v65
   =v66                                           -   Build for HVX v66

Do you think that's doable?

In D57030#1366269, @jhenderson wrote:

In D57030#1366088, @thopre wrote:

Thanks for the explanation. In that case I'd like to differentiate between the optional and non optional case.

1. For optional omitting the line is probably fine, or perhaps put a [default] instead of empty equal sign.
2. To handle the non optional case I think we should introduce single quotes after the equal sign to indicate that this is not an error but an empty value, ie. =''. Perhaps also put quote around other values for consistency.
   
   Does that make sense?

I'm afraid I'm not quite sure what you mean by the "optional and non optional case". Do you mean when ValueOptional is and is not used?

In D57030#1365979, @jhenderson wrote:

In D57030#1365762, @thopre wrote:

Do such sentinel trigger if not passing the equal sign (ie. when using only "-mhvx" in the example you quote) or only with equal sign without a value (one has to use "-mhvx=" to select the option)?

If the former I think this change is fine. I would also suggest adding an early continue when only the description is empty on line 1683 to avoid the empty '-'.

But if the latter (one has to do "-mhvx=" to select the option) I'm not sure hiding the option is right since it is a value (albeit empty) people can use.

Assuming ValueOptional is specified, then yes, you don't need the equal sign. I've written up a table below of different input options versus their behaviour to clarify things:

Code	--option	--option=	--option foo	--option=foo
default, no sentinel	not allowed	not allowed	allowed	allowed
default, sentinel	not allowed	allowed	allowed	allowed
ValueOptional, no sentinel	not allowed	not allowed	not allowed	allowed
ValueOptional, sentinel	allowed	allowed	not allowed	allowed

Do such sentinel trigger if not passing the equal sign (ie. when using only "-mhvx" in the example you quote) or only with equal sign without a value (one has to use "-mhvx=" to select the option)?

Clarify what happens in LookupOption when failing to match an option.

Fix comment in LookupOption for AlwaysPrefix early bail out

Take CommandLine option code into a separate patch

• Forbid AlwaysPrefix option to look for their value in following argument
• Add unit tests for changes to CommandLine

Add AlwaysPrefix option formatting flag and define FileCheck's -D option with it.
Add detection of missing variable name.

In D55940#1337957, @jdenny wrote:

In D55940#1337906, @probinson wrote:

This will permit -DVALUE= (with no value) is that a supported usage?

I think it should be. You could have a string that sometimes appears and sometimes is excluded entirely.

It also permits -D=10 which I daresay doesn't mean anything.

Agreed.

Document how to set numeric variable with -D option

FileCheck: Add support for numeric variables and expressions

LGTM. Thanks!

In D49084#1203189, @rnk wrote:

FWIW I had a use case for this recently, and found it hadn't landed. Is there anything blocking that you need help with?

Hi George,

• Change test for isSized() into assert
• Add test for multiple return values with matching input
• Rewrite hardfloat tests to return a struct to make register used predictable and not dependent on register allocator

Ping? This fix the bug reported in https://reviews.llvm.org/rL337903

In rL337903#281385, @thopre wrote:

In rL337903#281283, @tra wrote:

It appears that this patch crashes llvm under some circumstances.

Reproduction:

test.cc:

float asmfunc(int a, int b) {
  float read_value;
  asm("" : "=r"(read_value), "=r"(a));
  return read_value;
}
int B_X;
void kFCNorm(int a, float b) {
  B_X = asmfunc(a, B_X) * b;
}

$ clang -S -O3 test.cc -o - `
...
clang-7: <redacted>/repo/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:4489: llvm::SDValue llvm::SelectionDAG::getNode(unsigned int, const llvm::SDLoc &, llvm::EVT, llvm::SDValue, llvm::SDValue, const llvm::SDNodeFlags): Assertion `VT.isFloatingPoint() && "This operator only applies to FP types!"' failed.

I can reproduce, thanks. I'll see if I can find a fix today and if not will revert it and its follow-up patch.

Fixed in trunk

In rL337903#281283, @tra wrote:

It appears that this patch crashes llvm under some circumstances.

Reproduction:

test.cc:

float asmfunc(int a, int b) {
  float read_value;
  asm("" : "=r"(read_value), "=r"(a));
  return read_value;
}
int B_X;
void kFCNorm(int a, float b) {
  B_X = asmfunc(a, B_X) * b;
}

$ clang -S -O3 test.cc -o - `
...
clang-7: <redacted>/repo/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:4489: llvm::SDValue llvm::SelectionDAG::getNode(unsigned int, const llvm::SDLoc &, llvm::EVT, llvm::SDValue, llvm::SDValue, const llvm::SDNodeFlags): Assertion `VT.isFloatingPoint() && "This operator only applies to FP types!"' failed.

Rebase and rename testcase for consistency