[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 an inline comment as done.
agozillon added inline comments.


================
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:
> agozillon wrote:
> > 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.
> Consistency with Clang is a good idea 👍🏻  Though I would appreciate a test that demonstrates that this diagnostic is indeed issues when a non-existing files is specified :)
Thank you for pointing out a test was required! I've added one now :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148038