awarzynski added inline comments.
================ Comment at: flang/include/flang/Optimizer/Builder/Runtime/EnvironmentDefaults.h:7 +// +//===----------------------------------------------------------------------===// + ---------------- Could you document what these are? And what are they used for? ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:403-413 + if (const auto *arg = + args.getLastArg(clang::driver::options::OPT_fconvert_EQ)) { + auto parseConvertArg = [](const char *s) { + return llvm::StringSwitch<std::optional<const char *>>(s) + .Case("unknown", "UNKNOWN") + .Case("native", "NATIVE") + .Case("little-endian", "LITTLE_ENDIAN") ---------------- I'm OK with a lambda here, just pointing our that in other cases we added small hooks, e.g. `getOptimizationLevel`. I personally prefer hooks as this means that methods like `parseFrontendArgs` can be a bit shorter. But this is a very weak preference! ================ Comment at: flang/test/Driver/convert.f90:1 +! Ensure argument -fconvert=<value> works as expected. + ---------------- Could you be more specific? IIUC, this is more about making sure that the option parser works correctly and reports invalid usage of `-fconvert` as an error, right? ================ Comment at: flang/test/Driver/emit-mlir.f90:16 ! CHECK-NEXT: } +! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : !fir.ref<tuple<i[[int_size:.*]], !fir.ref<!fir.array<0xtuple<!fir.ref<i8>, !fir.ref<i8>>>>>> { +! CHECK-NEXT: %[[VAL_0:.*]] = fir.zero_bits !fir.ref<tuple<i[[int_size]], !fir.ref<!fir.array<0xtuple<!fir.ref<i8>, !fir.ref<i8>>>>>> ---------------- peixin wrote: > jpenix-quic wrote: > > peixin wrote: > > > jpenix-quic wrote: > > > > peixin wrote: > > > > > jpenix-quic wrote: > > > > > > peixin wrote: > > > > > > > Is it possible not to generated this global variable if > > > > > > > `fconvert=` is not specified? > > > > > > I'm not entirely sure--the issue I was running into was how to > > > > > > handle this in Fortran_main.c in a way which worked for all of > > > > > > GCC/Clang/Visual Studio (and maybe others?). I was originally > > > > > > thinking of doing this by using a weak definition of > > > > > > _QQEnvironmentDefaults set to nullptr so fconvert, etc. could > > > > > > override this definition without explicitly generating the fallback > > > > > > case. For GCC/clang, I think I could use __attribute__((weak)), but > > > > > > I wasn't sure how to handle this if someone tried to build with > > > > > > Visual Studio (or maybe another toolchain). I saw a few workarounds > > > > > > (ex: > > > > > > https://devblogs.microsoft.com/oldnewthing/20200731-00/?p=104024) > > > > > > but I shied away from this since it seems to be an undocumented > > > > > > feature (and presumably only helps with Visual Studio). > > > > > > > > > > > > Do you know of a better or more general way I could do this? (Or, > > > > > > is there non-weak symbol approach that might be better that I'm > > > > > > missing?) > > > > > How about generate one runtime function with the argument of > > > > > `EnvironmentDefaultList`? This will avoid this and using one extern > > > > > variable? > > > > > > > > > > If users use one variable with bind C name `_QQEnvironmentDefaults` > > > > > in fortran or one variable with name `_QQEnvironmentDefaults` in C, > > > > > it is risky. Would using the runtime function and static variable > > > > > with the type `EnvironmentDefaultList` in runtime be safer? > > > > Agreed that there are potential risks with the current approach > > > > (although, are the `_Q*` names considered reserved?). Unfortunately, I > > > > think generating a call to set the environment defaults requires > > > > somewhat significant changes to the runtime. The runtime reads > > > > environment variables during initialization in > > > > `ExecutionEnvironment::Configure` which is ultimately called from the > > > > "hardcoded" `Fortran_main.c` and I need to set the defaults before this > > > > happens. So, I believe I'd either have to move the initialization to > > > > `_QQmain` or make it so that `main` isn't hardcoded so that I could > > > > insert the appropriate runtime function. > > > > > > > > @klausler I think I asked you about this when I was first trying to > > > > figure out how to implement the environment defaults and you suggested > > > > I try the extern approach--please let me know if you have > > > > thoughts/suggestions around this! > > > This is what @klausler suggested: > > > ``` > > > Instead of adding new custom APIs that let command-line options control > > > behavior in a way that is redundant with the runtime environment, I > > > suggest that you try a more general runtime library API by which the main > > > program can specify a default environment variable setting, or a set of > > > them. Then turn the command-line options into the equivalent environment > > > settings and pass them as default settings that could be overridden by > > > the actual environment. > > > ``` > > > If I understand correctly, what I am suggesting match his comments. The > > > "main program" he means should be fortran main program, not the > > > `RTNAME(ProgramStart`. In your initial patch, you add the runtime > > > specified for "convert option". I think @klausler suggest you making the > > > runtime argument more general used for a set of runtime environment > > > variable settings, not restricted to "convert option". And that is what > > > you already added -- `EnvironmentDefaultList`. So, combining this patch > > > and your initial patch will be the solution. Hope I understand it > > > correctly. > > The issue I hit with the suggested approach is that in order to use the > > pre-existing runtime environment variable handling to set the internal > > state I need to set the environment variable defaults before the > > environment variables are read by the runtime. > > > > I might be misunderstanding/missing something, but given that the > > environment variables are read as part of `RTNAME(ProgramStart)` in `main` > > and the earliest I can place the call if I am generating it is `_QQmain`, I > > think that leaves three options: 1. don't hardcode `main` so that I can > > place the call early enough 2. delay or rerun the code [[ > > https://github.com/llvm/llvm-project/blob/c619d4f840dcba54751ff8c5aaafce0f173a4ad5/flang/runtime/environment.cpp#L50-L90 > > | here ]] that is responsible for initializing the runtime state so that > > it is called as part of `_QQmain` so I can insert my runtime function > > before it or 3. hardcode something like the `_QQEnvironmentDefaults` into > > Fortran_main.c so that the environment defaults are available early enough. > > Option 2 seems less than ideal to me, option 1 seems ideal but requires > > generating `main`, so option 3 is what I ended up going with. If options 1 > > or 2 would be preferable to what is currently implemented (or if there is > > another possibility I'm missing!) I'd be happy to switch and try to > > implement them. > > > What do other reviewers think? @klausler @awarzynski @kiranchandramohan > @clementval @jeanPerier > The current approach has two drawbacks: > 1. Add one extern variable `_QQEnvironmentDefaults` in runtime. > 2. Generate the variable during lowering even if there is no `-fconvert` > option. > > Can we accept this? Based on the analysis by @jpenix-quic, this is the path of least resistance and SGTM. It can always be refactored/improved in the future if need be. It would be helpful if this was documented somewhere. But I'm not sure whether there's a good landing space for this ATM. ================ Comment at: flang/test/Lower/environment-defaults.f90:1 +! RUN: bbc -emit-fir -o - %s | FileCheck %s + ---------------- Can you test with `flang-new` as well? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits