This is an archive of the discontinued LLVM Phabricator instance.

[ExecEngine] Add scanMemref* API and an `input-dir` option in JIT runner
Needs ReviewPublic

Authored by avarmapml on Jun 6 2022, 1:47 AM.

Details

Summary
  • This commit adds scanMemref* API for reading input data into memref during JIT execution.
  • It also adds input-dir option as part of mlir-cpu-runner tool to let the user mention the absolute path to the input directory from which the scanMemref* API will extract the data from.

Signed-off-by: Abhishek Varma <abhishek.varma@polymagelabs.com>

Diff Detail

Event Timeline

avarmapml created this revision.Jun 6 2022, 1:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 1:47 AM
avarmapml requested review of this revision.Jun 6 2022, 1:47 AM
avarmapml updated this revision to Diff 434402.Jun 6 2022, 2:01 AM

Trivial changes to test case

avarmapml edited the summary of this revision. (Show Details)Jun 6 2022, 6:45 AM
avarmapml retitled this revision from [ExecEngine] Add scan_memref API and an input-dir option in JIT runner to [ExecEngine] Add scanMemref* API and an `input-dir` option in JIT runner.
avarmapml updated this revision to Diff 435499.Jun 9 2022, 4:40 AM

Updated integer to string conversion logic.

  • Build fails for Windows because to_string isn't available as a member of std.
  • Implemented a trivial integer->string conversion to tackle the same.
avarmapml updated this revision to Diff 435517.Jun 9 2022, 5:57 AM

Shifted execution test case within mlir/test/mlir-cpu-runner

Can someone review this patch?
I've addressed the Windows build failures resulting earlier.

@bondhugula

@ftynse @herhut Can you review this patch?

@herhut - would you be able to also review this?

mlir/include/mlir/ExecutionEngine/RunnerUtils.h
303

We should have these emitted to stderr?

303

Opening file or Opened file?
Nit: no space before colon.

308

Nit: Full stop at the end.

avarmapml marked 3 inline comments as done.

Trivial nit changes.

mehdi_amini added inline comments.Jun 28 2022, 3:15 AM
mlir/include/mlir/ExecutionEngine/RunnerUtils.h
38

Any reason you're not using the LLVM streams here?

43

That seems like quite a hack to pass state around to me, using an environment variable may an alternative?

avarmapml updated this revision to Diff 440666.Jun 28 2022, 9:47 AM
avarmapml marked 2 inline comments as done.

Using environment variable and llvm::itostr

  • Modified the code to incorporate using environment variable to pass the input directory path to the runtime utility.
  • Using llvm::itostr instead of sstream.
mlir/include/mlir/ExecutionEngine/RunnerUtils.h
38

I was using sstream to get the int->string conversion go smoothly.
Wasn't aware about itostr as part of llvm/Support/StringExtras.h - but now using that instead.
Thanks for the nudge here!

43

Thanks for suggesting this! I've made the appropriate changes.

mehdi_amini added inline comments.Jun 28 2022, 1:10 PM
mlir/include/mlir/ExecutionEngine/RunnerUtils.h
43

Nit: constexpr char * instead of define.

295

LLVM should have enough file manipulation routine to avoid std::fstream here as well I think.

298

Left over debugging.

304

Can you move all the part of the implementation that is not dependent on T into a non-templated function that would be in the .cpp file? That would avoid exposing all of the file manipulation and the env var in the header.

mlir/lib/ExecutionEngine/JitRunner.cpp
389

I would hope we could remove all changes in this file: why do we even need a command line flag still?

avarmapml updated this revision to Diff 441644.Jul 1 2022, 2:14 AM
avarmapml marked 5 inline comments as done.

Shifted opening file utility to RunnerUtils.cpp

  • Shifted opening file utility based on environment variable to RunnerUtils.cpp.
  • Handled other review comments.
avarmapml updated this revision to Diff 441645.Jul 1 2022, 2:18 AM

Shifted the file opening utility and environment variable

mlir/include/mlir/ExecutionEngine/RunnerUtils.h
295

I tried exploring/implementing the code with both raw_fd_stream as well as llvm/Support/FileSystem.h but I believe they require the char * as the buffer input.
I guess using LLVM's file manipulation routines would thus involve some local manipulations perhaps.
std::fstream with that respect seems quite cleaner/ "friendly" to me.

Can you perhaps give me some pointers here perhaps for me to try?

298

I thought it'd be nice to let the user know the status of scanning each argument during runtime, so included that (the position of the statement itself should've been right after open though).

But I've removed it now in this revision and keeping statements pertaining just the beginning and completion of scanMemref*.

mlir/lib/ExecutionEngine/JitRunner.cpp
389

Well, if the user is indeed creating an environment variable prior to calling mlir-cpu-runner, then we don't require the flag as you rightly mentioned.

But I see two reasons for still requiring the flag and then internally creating the environment variable :-

  1. Perhaps user can mess up naming the environment variable correctly?
  2. Also, the test case I've included in this revision needs to know where the input files are. Not sure how we're to set environment variable in that case.

Keeping the flag makes the execution process similar to both the user and the lit tests.

Let me know your thoughts here, perhaps I'm missing something.

mehdi_amini added inline comments.Jul 3 2022, 10:28 AM
mlir/include/mlir/ExecutionEngine/RunnerUtils.h
295

All of LLVM tools (like mlir-opt, etc.) are using LLVM steams and they are still loading files: so it is definitely possible...
Quickly looking at mlir-opt implementation, the input file is passed to mlir::openInputFile which is implemented in mlir/lib/Support/FileUtilities.cpp.

There is a constructor for raw_fd_stream that is: raw_fd_stream(StringRef Filename, std::error_code &EC); : seems like this takes a FileName?

mlir/lib/ExecutionEngine/JitRunner.cpp
389

I thought of an environment variable to keep this entirely out of the JitRunner.cpp actually. I didn't quite get what is your problem with the test though?

Alternatively, we could scrap all of this and use an API instead: the IR call could pass in the directory where to load a memref (environment variables are a bit hacky anyway)