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,
----------------
dblaikie wrote:
> 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.
> 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.
Oh, I get it. Thanks.
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
