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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
