This is an archive of the discontinued LLVM Phabricator instance.

[fir] Add test for FIR types conversion
ClosedPublic

Authored by clementval on Nov 5 2021, 7:58 AM.

Details

Summary

Add a separate file to test FIR types conversion to LLVM types.
Conversion comes from flang/lib/Optimizer/CodeGen/TypeConverter.h

This patch is part of the upstreaming effort from fir-dev branch.

Diff Detail

Event Timeline

clementval created this revision.Nov 5 2021, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2021, 7:58 AM
clementval requested review of this revision.Nov 5 2021, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2021, 7:58 AM
awarzynski added inline comments.
flang/test/Fir/types-to-llvm.fir
10

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.

clementval added inline comments.Nov 5 2021, 8:48 AM
flang/test/Fir/types-to-llvm.fir
10

Why not all on a single line:

// CHECK: foo0(%{{.*}}: !llvm.array<10 x array<10 x i64>>)

I find it less "verbose" personnally.

awarzynski added inline comments.Nov 5 2021, 9:32 AM
flang/test/Fir/types-to-llvm.fir
10

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

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:

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

func private @foo0(%arg0: !fir.array<10x10xi64>)
// CHECK: foo0(!llvm.array<11 x array<10 x i64>>

This will produce the following error:

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.

clementval updated this revision to Diff 385179.Nov 5 2021, 1:47 PM

Chnage check lines

clementval marked 2 inline comments as done.Nov 5 2021, 1:48 PM
This revision is now accepted and ready to land.Nov 5 2021, 3:07 PM
awarzynski accepted this revision.Nov 6 2021, 3:14 AM

LGTM, thanks for the updates!

This revision was automatically updated to reflect the committed changes.