This is an archive of the discontinued LLVM Phabricator instance.

[mlir] add support for verification in integration tests
ClosedPublic

Authored by gysit on Feb 9 2021, 2:16 AM.

Details

Summary

The patch extends the runner utils by verification methods that compare two memrefs. The methods compare the content of the two memrefs and print success if the data is identical up to a small numerical error. The methods are meant to simplify the development of integration tests that for example compare optimized and unoptimized code paths (cf. the updates to the linalg matmul integration tests).

Diff Detail

Event Timeline

gysit created this revision.Feb 9 2021, 2:16 AM
gysit requested review of this revision.Feb 9 2021, 2:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2021, 2:16 AM

Thanks for adding this @gysit !
Can you please also address the lints?

mlir/include/mlir/ExecutionEngine/RunnerUtils.h
294

We should probably avoid printing to stdout in general.
How about we make this return an error code i8 and just print that in MLIR.
We can print errors to stderr.

mlir/integration_test/Dialect/Linalg/CPU/benchmark_matmul.mlir
61

Can we move this closer to its uses and "scope it" more tightly?

alloc
fill
run
test
print
dealloc
98

let's please make this return an i8 that we just vector.print and //CHECK: 0

gysit updated this revision to Diff 322349.Feb 9 2021, 5:08 AM

Avoid std::cout and return number of errors instead of printing success or failure. Restructured the unit tests to limit the scope of the verification code. Also tried to address the lint issues. Howerever the _mlir_ciface_ perfix is fix as far as I understand.

gysit marked 3 inline comments as done.Feb 9 2021, 5:12 AM

@nicolasvasilache the updated revision should address your comments. The linter may not be happy with the function naming (_mlir_ciface) and I somewhat prefer the older function names.

nicolasvasilache accepted this revision.Feb 9 2021, 5:25 AM
This revision is now accepted and ready to land.Feb 9 2021, 5:25 AM
This revision was automatically updated to reflect the committed changes.

Could you please add #include<algorithm> for RunnerUtils.h? MSVC requires it for std::max/min, resulting in this http://lab.llvm.org:8011/#/builders/13/builds/4283/steps/6/logs/stdio failure. Thanks!

To be clear, the windows mlir buildbot is broken because of this change:

http://lab.llvm.org:8011/#/builders/13/builds/4284

Perhaps you are not getting the notifications?

gysit added a comment.Feb 9 2021, 10:23 AM

To be clear, the windows mlir buildbot is broken because of this change:

http://lab.llvm.org:8011/#/builders/13/builds/4284

Perhaps you are not getting the notifications?

sorry did not see it immediately. Reverted the commit for now.

gysit updated this revision to Diff 322452.Feb 9 2021, 11:20 AM

Added missing algorithms header to fix Windows build.