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,
----------------
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.
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
