ChuanqiXu added inline comments.
================
Comment at: clang/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp:5
// CHECK-DAG: @extern_var_exported = external {{(dso_local )?}}global
-// CHECK-DAG: @inline_var_exported = linkonce_odr {{(dso_local )?}}global
+// CHECK-DAG: @inline_var_exported = available_externally {{(dso_local
)?}}global
// CHECK-DAG: @const_var_exported = available_externally {{(dso_local
)?}}constant i32 3,
----------------
ChuanqiXu wrote:
> urnathan wrote:
> > ChuanqiXu wrote:
> > > urnathan wrote:
> > > > ChuanqiXu wrote:
> > > > > urnathan wrote:
> > > > > > I don;t think this is correct. That should still be a linkonce
> > > > > > odr, otherwise you'll get conflicts with other module
> > > > > > implementation units.
> > > > > It is still linkonce_odr in the module it get defined. See the new
> > > > > added test case: inline-variable-in-module.cpp for example. The
> > > > > attribute `available_externally` is equivalent to external from the
> > > > > perspective of linker. See
> > > > > https://llvm.org/docs/LangRef.html#linkage-types. According to
> > > > > [dcl.inline]p7, inline variable attached to named module should be
> > > > > defined in that domain. Note that the variable attached to global
> > > > > module fragment and private module fragment shouldn't be accessed
> > > > > outside the module, so it implies that all the variable defined in
> > > > > the module could only be defined in the module unit itself.
> > > > There's a couple of issues with this. module.cppm is emitting a
> > > > (linkonce) definition of inlne_var_exported, but only because it itself
> > > > is ODR-using that variable. If you take out the ODR-use in
> > > > noninline_exported, there is no longer a symbol emitted.
> > > >
> > > > But, even if you forced inline vars to be emitted in their
> > > > defining-module's interface unit, that would be an ABI change. inline
> > > > vars are emitted whereever ODR-used. They have no fixed home TU. Now,
> > > > we could alter the ABI and allow interface units to define a home
> > > > location for inline vars and similar entities (eg, vtables for keyless
> > > > classes). But we'd need buy-in from other compilers to do that.
> > > >
> > > > FWIW such a discussion did come up early in implementing modules-ts,
> > > > but we decided there was enough going on just getting the TS
> > > > implemented. I'm fine with revisiting that, but it is a more
> > > > significant change.
> > > >
> > > > And it wouldn't apply to (eg) templated variables, which of course
> > > > could be instantiated anywhere.
> > > Oh, now the key point here is what the correct behavior is instead of the
> > > patch. Let's discuss it first.
> > >
> > > According to [[ http://eel.is/c++draft/dcl.inline#7 | [dcl.inline]p7 ]],
> > > > If an inline function or variable that is attached to a named module is
> > > > declared in a definition domain, it shall be defined in that domain.
> > >
> > > I think the intention of the sentence is to define inline variable in the
> > > module interface. So if it is required by the standard, I think other
> > > compiler need to follow up. As I described in the summary, it might be a
> > > difference between C++20 module and ModuleTS. Do you think it is
> > > necessary to send the question to WG21? (I get the behavior from reading
> > > the words. Maybe I misread or the word is not intentional).
> > >
> > > Maybe the ABI standard group need to discuss what the linkage should be.
> > > Now it may be weak_odr or linkonce_odr. It depends on how we compile the
> > > file. If we compile the .cppm file directly, it would be linkonce_odr.
> > > And if we compile it to *.pcm file first, it would be weak_odr. I have
> > > registered an issue for this:
> > > https://github.com/llvm/llvm-project/issues/53838.
> > > Oh, now the key point here is what the correct behavior is instead of the
> > > patch. Let's discuss it first.
> > >
> > > According to [[ http://eel.is/c++draft/dcl.inline#7 | [dcl.inline]p7 ]],
> > > > If an inline function or variable that is attached to a named module is
> > > > declared in a definition domain, it shall be defined in that domain.
> > >
> > > I think the intention of the sentence is to define inline variable in the
> > > module interface. So if it is required by the standard, I think other
> > > compiler need to follow up. As I described in the summary, it might be a
> > > difference between C++20 module and ModuleTS. Do you think it is
> > > necessary to send the question to WG21? (I get the behavior from reading
> > > the words. Maybe I misread or the word is not intentional).
> >
> > You are reading more into the std than it says. The std specifies what
> > /source code/ is meaningful. It says nothing about how a computation
> > system might represent the program in another form. Most of the latter,
> > for ahead-of-time translation, is at the discretion of compiler
> > implementors. Part of that is the domain of the ABI, which specifies an
> > interface to which different compilers may target, and then have
> > compatibility at the object-file boundary.
> >
> > > Maybe the ABI standard group need to discuss what the linkage should be.
> >
> > Correct. And right now there is no consensus to do anything different with
> > such entities.
> > The ABI (http://itanium-cxx-abi.github.io/cxx-abi/abi.html) 5.2 documents
> > such vague-linkage entities. That section would need changes to bless what
> > you are trying to do.
> >
> >
> > You are reading more into the std than it says. The std specifies what
> > /source code/ is meaningful. It says nothing about how a computation system
> > might represent the program in another form. Most of the latter, for
> > ahead-of-time translation, is at the discretion of compiler implementors.
> > Part of that is the domain of the ABI, which specifies an interface to
> > which different compilers may target, and then have compatibility at the
> > object-file boundary.
>
> OK, your words make sense. In fact, I don't care much about whether or not
> could we define `inline variable` in the module unit. The problem I tried to
> solve is about `the definition static variable in module`. We couldn't run a
> simple hello world example if we don't solve it.
>
> What I care about is where should we define inline function. I want to define
> inline function in the module unit it get declared. And my theory comes from
> [dcl.inline]p7. And our experiments show that it is the key reason why module
> could speed up compilation. Our data shows that the compilation could speed
> up about 40% for the feature. Since most of the time consumed in compilation
> spent on the middle end, it is really not significant to save the time in
> frontend. So it matters a lot if we could avoid compiling same functions in
> middle end.
>
> Originally, I thought I am doing right. But from your words, we couldn't do
> this until the ABI standard group get in consensus, right?
>
> Finally, I feel it is odd about [dcl.inline]p7. Since if it is not for
> implementors, I feel it is meaningless for users.
Or given that the CXXABI doesn't discuss the case about inline function in
named module. Could we think it is a free space? Another question maybe where
could we ask for opinion? WG21 or Itanium CXXABI group?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119409/new/
https://reviews.llvm.org/D119409
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
