This is an archive of the discontinued LLVM Phabricator instance.

Make mlir-opt --show-dialects option print on a single line
ClosedPublic

Authored by mehdi_amini on Mar 6 2023, 9:11 AM.

Details

Summary

Also add a helpful message that mlir-opt continues with normal processing to
avoid folks waiting while mlir-opt is stuck on stdin.

Diff Detail

Event Timeline

mehdi_amini created this revision.Mar 6 2023, 9:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2023, 9:11 AM
mehdi_amini requested review of this revision.Mar 6 2023, 9:11 AM

Fix test, make it all in one line

mehdi_amini added inline comments.Mar 6 2023, 9:16 AM
mlir/test/mlir-opt/commandline.mlir
43

Actually the help message won't work here, I need more work on the refactoring...

gflegar added inline comments.Mar 6 2023, 10:21 AM
mlir/test/mlir-opt/commandline.mlir
1

It's actually quite interesting that this test works. For us internally, this is the one that break (I first assumed that it was some internal test, but it's actually this upstream test that hangs). Maybe a different way of running lit is causing this. But it would be much nicer if the run line was echo "" | mlir-opt --show-dialects | FileCheck %s to make sure it finishes.

gflegar added inline comments.Mar 6 2023, 10:38 AM
mlir/test/mlir-opt/commandline.mlir
1
mehdi_amini added inline comments.Mar 6 2023, 10:40 AM
mlir/test/mlir-opt/commandline.mlir
1

I suspect you have some downstream specific runner that don't close stdin properly.
But I'm not sure why you push fixes that don't fix anything upstream actually?

rkayaith added inline comments.
mlir/test/mlir-opt/commandline.mlir
1

It's actually quite interesting that this test works. For us internally, this is the one that break (I first assumed that it was some internal test, but it's actually this upstream test that hangs). Maybe a different way of running lit is causing this. But it would be much nicer if the run line was echo "" | mlir-opt --show-dialects | FileCheck %s to make sure it finishes.

See this thread: https://discourse.llvm.org/t/lit-runner-passing-empty-stdin-to-tool-makes-incorrect-tests-pass/65672
The current behaviour is that tests get run with stdin as empty string, and a change to disable that seems stalled: https://reviews.llvm.org/D135067

Split the message in a subsequent patch

gflegar accepted this revision.Mar 7 2023, 1:17 AM
This revision is now accepted and ready to land.Mar 7 2023, 1:17 AM