peixin added inline comments.
================
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>>>>>>
----------------
awarzynski wrote:
> 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.
How about adding FIXME when generating the variables during lowering
(flang/lib/Lower/Bridge.cpp).
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
