[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: clang/include/clang/Driver/Options.td:6558
   HelpText<"Path to the IR file produced by the frontend for the host.">,
-  Flags<[CC1Option, NoDriverOption]>;
+  Flags<[CC1Option, FC1Option, NoDriverOption]>;
 
----------------
With both options using identical `Flags`, could you use `let Flags = `?


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:144
+  // adds other secondary input (e.g. device bitcode files for embedding to the
+  // -fembed-offload-object argument or the host ir file for proccessing
+  // during device compilation to the fopenmp-host-ir-file-path argument via
----------------



================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:146
+  // during device compilation to the fopenmp-host-ir-file-path argument via
+  // OpenMPDeviceInput) This is condensed logic from the Clang driver
+  // for pushing on further input arguments needed for offloading
----------------
[nit] You could refer to a specific method in Clang (I did that in the past in a few places)


================
Comment at: flang/include/flang/Frontend/LangOptions.h:18
 
+#include <string>
 namespace Fortran::frontend {
----------------
[nit] Empty line between


================
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)
----------------
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? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148038