[PATCH] D113283: [fir] Add test for FIR types conversion

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
Comment at: flang/test/Fir/types-to-llvm.fir:9
+func private @foo0(%arg0: !fir.array<10x10xi64>)
+// CHECK: !llvm.array<10 x array<10 x i64>>
+func private @foo1(%arg0: !fir.array<?xf32>)
----------------
clementval wrote:
> awarzynski wrote:
> > Could you use this idiom for other cases too? Otherwise, there's no guarantee that a particular `CHECK` line matches on the line that we want it to match.
> Why not all on a single line:
> 
> ```
> // CHECK: foo0(%{{.*}}: !llvm.array<10 x array<10 x i64>>)
> ```
> I find it less "verbose" personnally. 
It's more compact in the test file, yes, but less helpful when a test is failing (and that's when you want to be able to analyze the output efficiently).

 Consider two examples, both are based on the first test from this file and both are made to fail (see `11` in the argument type):

**VERBOSE TEST AND CLEAR LOG**
```lang=llvm
func private @foo0(%arg0: !fir.array<10x10xi64>)
// CHECK-LABEL: foo0
// CHECK-SAME: !llvm.array<11 x array<10 x i64>>
```

This will give the following error:
```lang=bash
flang/test/Fir/types-to-llvm.fir:10:16: error: CHECK-SAME: expected string not found in input
// CHECK-SAME: !llvm.array<11 x array<10 x i64>>
               ^
<stdin>:2:17: note: scanning from here
 llvm.func @foo0(!llvm.array<10 x array<10 x i64>>) attributes {sym_visibility = "private"}
                ^
<stdin>:2:18: note: possible intended match here
 llvm.func @foo0(!llvm.array<10 x array<10 x i64>>) attributes {sym_visibility = "private"}
                 ^
```

It is very clear that it's the `CHECK-SAME` line that's failing. Also, `^` clearly indicates that the label was matched correctly and that you should be checking the argument type.

**COMPACT TEST AND VAGUE LOG**
```lang=llvm
func private @foo0(%arg0: !fir.array<10x10xi64>)
// CHECK: foo0(!llvm.array<11 x array<10 x i64>>
```

This will produce the following error:
```lang=bash
flang/test/Fir/types-to-llvm.fir:10:11: error: CHECK: expected string not found in input
// CHECK: foo0(!llvm.array<11 x array<10 x i64>>
          ^
<stdin>:1:1: note: scanning from here
module {
^
<stdin>:2:13: note: possible intended match here
 llvm.func @foo0(!llvm.array<10 x array<10 x i64>>) attributes {sym_visibility = "private"}
            ^
```

This time it's not clear at all whether it's failing to match
* the function name,
* the argument types.

Basically, it could be anything on that line, right?

I appreciate that in this case the difference is rather subtle. And it's easy to spot the failure anyway. But it becomes quite helpful when dealing with more complex labels, types and tests with multiple lines (like in this file in which there are only two logical file blocks separated with `-----`). In particular, in tests written by other developers that one is not familiar with. 

Also, when things are failing in an upstream buildbot, it can be incredibly helpful to be able to triage quickly. And for that, good failure log is key.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113283/new/

https://reviews.llvm.org/D113283