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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski 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)
----------------
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 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148038