dblaikie 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:
> 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?
> Finally, I feel it is odd about [dcl.inline]p7. Since if it is not for
> implementors, I feel it is meaningless for users.
Presumably that means that a user can't declare an inline function in a module,
and define it somewhere else (like in a private implementation unit) - they
must define it in the same definition domain it is declared. That's a concrete
requirement for the user, irrespective of what object-file-level implementation
strategy (where the definition gets emitted, what linkage is used, etc) is used.
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
