[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon marked 4 inline comments as done.
agozillon added a comment.

Addressed 4/5 of your comments in the latest patch @awarzynski, I've replied to the final one to give a little more information to add to the final decision you make!



================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:730
+        res.getLangOpts().OMPHostIRFile = arg->getValue();
+        if (!llvm::sys::fs::exists(res.getLangOpts().OMPHostIRFile))
+          diags.Report(clang::diag::err_drv_omp_host_ir_file_not_found)
----------------
awarzynski wrote:
> I think that this is expecting a bit too much from `CompilerInvocation`. Instead, whatever is going to use this file should complain if the file does not exist (`CompilerInvocation` is merely a messenger here, and file I/O can be quite expensive, hence my reservations).
> 
> Is it possible to create a test for "invalid file path" wherever this becomes relevant? 
I believe the follow up patch I have here which uses the file, emits an error if it can't find it: https://reviews.llvm.org/D148370 
However, in this case it's an assert rather than more useful Diagnostics unfortunately. 

Although, this segment of code does currently just mimic what Clang does in it's own 
`CompilerInvocation`: https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInvocation.cpp#L3883 

So it depends if we wish to try to maintain the status quo for the shared argument across the compiler frontends! 

So whichever you prefer, I am happy to remove the check at this level and let the lowering handle the problem if it arises :-) but I do like sharing the commonality with Clang where possible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148038/new/

https://reviews.llvm.org/D148038