spirv-val: Allow spir-val to process directories

[IgWod-IMG]

Hey, I've been looking into extending spirv-val so it can take directories as an input, and I'd like some feedback from the project maintainers on whether they would be happy with such change, and what's their preferred approach for it. For a bit of context (more at the end), this change is needed to improve how we validate SPIR-V MLIR dialect in the LLVM Project, where we need to pass a directory of SPIR-V binaries to spirv-val.

As currently spirv-val only accepts a single file, I see 2 ways to extended it:

1. [preferred] Allow spir-val to accept directories as an input. The tool will internally check if the passed name is a directory or a file, and if it's a directory, it will process all files in that directory (potentially filtered on the magic number to allow mix of files in the directory). This would be fully transparent to the user.
2. Same as above but we would have a flag (e.g., --validate-dir) indicating that the passed name is a directory, so passing a directory cannot be done accidently.

Once a consensus is reached, I'll work on the implementation and open a PR. I'm also happy to provide any more information.

For more context, in SPIR-V MLIR we have many tests that serialize SPIR-V MLIR into SPIR-V binaries and deserializes it back into MLIR (roundtrip test), however this does not guarantee that the SPIR-V modules that go through the roundtrip adhere to the spec and validation rules. We now want to extend those tests so after serializing MLIR into SPIR-V, spirv-val is run on the binary to confirm that the SPIR-V MLIR module is a valid SPIR-V. All good for now. The problem that we're facing is that a single MLIR test can contain multiple SPIR-V modules. Until recently all those modules were serialized into a single file, making it unusable with SPIR-V tools (multiple modules in a single file are not allowed). To that end we extended mlir-translate to create a separate .spv file for each module in the test (llvm/llvm-project#152678). Now we're able to run spirv-val on those files using xargs (llvm/llvm-project#153227), e.g., ls %t/module*.spv | xargs -I{} spirv-val {}. This however depends on having a shell and external utilities and we would like to avoid it, and pass the directory directly to spirv-val so the invocation simplifies to spirv-val %t/.

Many thanks,
Igor

cc @kuhar @davidegrohmann

[arcady-lunarg]

Another option would be to modify spirv-val to accept more than one file on the command line, which would put the burden of filtering and directory traversal and so on outside spirv-tools. But it does come with the limitation that you can only process as many files as fit on a command line, so you might still end up needing xargs or similar if you have a lot of files.

[kuhar]

But it does come with the limitation that you can only process as many files as fit on a command line, so you might still end up needing xargs or similar if you have a lot of files.

This is something we would like to avoid because we would have to rely on the shell being available (which is not a given on Windows). Having it handled on the spirv-tools side would make it easier to guarantee test coverage on all platforms in MLIR.

[alan-baker]

How do you envision the validation would occur? Would it stop on first failure (like it does for a single module), or would it process each file no matter what?

I'm not opposed to supporting option 1, though I'm not a huge fan of processing all files. I'd probably default to all .spv files and consider an option to specify which extensions to include.

[kuhar]

How do you envision the validation would occur? Would it stop on first failure (like it does for a single module), or would it process each file no matter what?

I'm fine with either, but I imagine the exit code to be non-zero if any of the inputs fails to validate

[IgWod-IMG]

How do you envision the validation would occur? Would it stop on first failure (like it does for a single module), or would it process each file no matter what?

I'm fine with either, but I imagine the exit code to non-zero if any of the inputs fails to validate

So far in my prototype implementation I process all files (no early exit), printing the validation error for any file that failed as I go and, in the end, return non-zero if any file failed. That probably would be a desired behaviour so you can see and fix all failures at the same time. Otherwise, you need to keep re-running the tool. But if the other option is preferred, I can work with that.

The only problem I have encountered so far is attributing validation errors to a file. The internals of the validator have (as far as I can see) no concept of file name, so we either need to fix that, so that both file name and line are printed in the error message or find some workaround.

I'm not opposed to supporting option 1, though I'm not a huge fan of processing all files. I'd probably default to all .spv files and consider an option to specify which extensions to include.

At least in MLIR validation we generate files with .spv extension so that works for us, and I agree it is a cleaner solution. Extra option is a good idea, but maybe that's something we can add later if the need arises.

[kuhar]

The only problem I have encountered so far is attributing validation errors to a file. The internals of the validator have (as far as I can see) no concept of file name, so we either need to fix that, so that both file name and line are printed in the error message or find some workaround.

We can always print the filename before we validate -- this way you can check that it actually picked up all the files you expected it to process

[IgWod-IMG]

The only problem I have encountered so far is attributing validation errors to a file. The internals of the validator have (as far as I can see) no concept of file name, so we either need to fix that, so that both file name and line are printed in the error message or find some workaround.

After a bit more of investigation it turned out filenames can be easily printed within error messages by registering a custom message consumer with SetMessageConsumer.

I'd be inclined to only print anything when validation fails, so no output implies that validation was successful --- this is consistent with the current behaviour. Then we can have a verbose flag that will print every filename as it's being processed.

[alan-baker]

I process all files (no early exit), printing the validation error for any file that failed as I go and, in the end, return non-zero if any file failed.

This is reasonable.

After a bit more of investigation it turned out filenames can be easily printed within error messages by registering a custom message consumer with SetMessageConsumer.

I'd be inclined to only print anything when validation fails, so no output implies that validation was successful --- this is consistent with the current behaviour. Then we can have a verbose flag that will print every filename as it's being processed.

I don't think such a flag is necessary. The message consumer is enough.

[IgWod-IMG]

I have opened a pull request for this: #6292