[PATCH] D119012: [flang][driver] Add support for the `-emit-llvm` option

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

I believe that Windows failures are due to the missing support here: https://github.com/llvm/llvm-project/blob/81cde474e2c5a6280cb693b777ddcf473825ae8a/flang/lib/Optimizer/CodeGen/Target.cpp#L290. I can disable the LIT test on Windows, but I'm not sure how to do it for unit tests.



================
Comment at: flang/lib/Frontend/FrontendActions.cpp:421
+  // Set-up the MLIR pass manager
+  fir::setTargetTriple(*mlirModule_, "native");
+  auto &defKinds = ci.invocation().semanticsContext().defaultKinds();
----------------
kiranchandramohan wrote:
> awarzynski wrote:
> > rovka wrote:
> > > Nit: Should we assert that mlirModule exists?
> > > Also, why doesn't it already have a triple and a kind mapping?
> > > Nit: Should we assert that mlirModule exists?
> > 
> > We could do, yes. However, `mlirModule` is created in `CodeGenAction::BeginSourceFileAction`. If that method fails, the driver should stop immediately. So perhaps that would be a better for place for an assert?
> > 
> > On a related note, `mlirModule` is obtained via [[ https://github.com/llvm/llvm-project/blob/79b3fe80707b2eb9a38c1517a829fb58062fb687/flang/include/flang/Lower/Bridge.h#L67 | LoweringBridge::getModule ]]. But if the corresponding module is `nullptr` than that getter should probably assert. See https://reviews.llvm.org/D119133.
> > 
> > >  Also, why doesn't it already have a triple and a kind mapping?
> > Good catch, this is not needed yet. I didn't notice this when going over `tco`.
>  D118985 creates a bridge with an empty triple. The patch here was switching it to "native". Please cross-check what the expected behaviour.
> 
> ```
>   lower::LoweringBridge lb = Fortran::lower::LoweringBridge::create(*mlirCtx,
>       defKinds, ci.invocation().semanticsContext().intrinsics(),
>       ci.parsing().allCooked(), "", kindMap);
> ```
> Please cross-check what the expected behaviour.
That's a good point, Kiran! Thanks :) 

The constructor for `LoweringBridge` will call [[ https://github.com/llvm/llvm-project/blob/81cde474e2c5a6280cb693b777ddcf473825ae8a/flang/lib/Optimizer/Support/FIRContext.cpp#L21-L24 | fir::setTargetTriple ]]. This, in turn, will call [[ https://github.com/llvm/llvm-project/blob/81cde474e2c5a6280cb693b777ddcf473825ae8a/flang/lib/Optimizer/Support/FIRContext.cpp#L53-L62 | fir::determineTargetTriple ]], which (for an empty triple) selects the target as follows:
```lang=cpp
  if (triple.empty() || triple == "default")
    return llvm::sys::getDefaultTargetTriple()
```
AFAIK, `native` and `default` are compatible (I couldn't find any indication otherwise). So, empty triple above and "native" will lead to the same thing. So we are still just using `native` here.

This made me realise though that we should be explicit in D118985  and set the triple there. I'll update it shortly.


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:440
+  // Translate to LLVM IR
+  auto optName = mlirModule->getName();
+  llvmCtx = std::make_unique<llvm::LLVMContext>();
----------------
kiranchandramohan wrote:
> Nit: Remove auto.
Sure! I will also rename this as `moduleName`.


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:471
+
+  if (!ci.IsOutputStreamNull()) {
+    llvmModule->print(
----------------
rovka wrote:
> Nit: Can this be folded into the above if? (I.e. just write to os if you managed to create it, and add an else branch for the other case)
Good suggestion, thanks!

I recall having a good reason to write it like this, but don't remember what it was. Let me simplify!


================
Comment at: flang/unittests/Frontend/FrontendActionTest.cpp:176
+  // stream, as opposed to an actual file (or a file descriptor).
+  llvm::SmallVector<char, 256> outputFileBuffer;
+  std::unique_ptr<llvm::raw_pwrite_stream> outputFileStream(
----------------
kiranchandramohan wrote:
> Nit: Is the size important?
No, thanks for noting this!

In fact, the generated output is larger than 256 characters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119012