Re: r295252 - [Modules] Consider enable_if attrs in isSameEntity.

[George Burgess IV via cfe-commits]

WFM; added them to ExtParameterInfo in r296076. Thanks for the idea!

On Wed, Feb 15, 2017 at 5:44 PM, Richard Smith 
wrote:

> On 15 February 2017 at 17:32, George Burgess IV <
> george.burgess...@gmail.com> wrote:
>
>> I remember that we wanted to pretend that pass_object_size isn't a part
>> of the FunctionType during the review that added it, though.
>>
>
> I remember we wanted to not add extra fake "parameters" to the
> FunctionType to model pass_object_size. I don't remember whether or why we
> wanted it to not be part of the function type at all -- on reflection, it
> seems as much a part of the type as, say, a calling convention (which it
> is, in some sense).
>
>
>> Do you think that would be better than serializing parameters before we
>> serialize template info? AFAICT, we only do merging after we start reading
>> the template info, so I can't immediately see why that wouldn't work.
>>
>
> I would be concerned about the possibility of that introducing dependency
> cycles into the deserialization process. For instance, merging default
> arguments for function parameters may require us to have already merged the
> function itself into its redeclaration chain (we don't currently model that
> quite correctly, so we probably won't hit it today).
>
>
>> On Wed, Feb 15, 2017 at 4:55 PM, Richard Smith 
>> wrote:
>>
>>> On 15 February 2017 at 14:43, George Burgess IV via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Author: gbiv
 Date: Wed Feb 15 16:43:27 2017
 New Revision: 295252

 URL: http://llvm.org/viewvc/llvm-project?rev=295252=rev
 Log:
 [Modules] Consider enable_if attrs in isSameEntity.

 Two functions that differ only in their enable_if attributes are
 considered overloads, so we should check for those when we're trying to
 figure out if two functions are mergeable.

 We need to do the same thing for pass_object_size, as well. Looks like
 that'll be a bit less trivial, since we sometimes do these merging
 checks before we have pass_object_size attributes available (see the
 merge checks in ASTDeclReader::VisitFunctionDecl that happen before we
 read parameters, and merge checks in calls to ReadDeclAs<>()).

>>>
>>> Perhaps the best way to tackle this would be to track the presence of
>>> pass_object_size as part of the function type (in the ExtParameterInfo data
>>> on the function type).
>>>
>>> Added:
 cfe/trunk/test/Modules/Inputs/overloadable-attrs/
 cfe/trunk/test/Modules/Inputs/overloadable-attrs/a.h
 cfe/trunk/test/Modules/Inputs/overloadable-attrs/module.modulemap
 cfe/trunk/test/Modules/overloadable-attrs.cpp
 Modified:
 cfe/trunk/lib/Serialization/ASTReaderDecl.cpp

 Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
 URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serializat
 ion/ASTReaderDecl.cpp?rev=295252=295251=295252=diff
 
 ==
 --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
 +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Feb 15 16:43:27
 2017
 @@ -2656,6 +2656,44 @@ static bool isSameTemplateParameterList(
return true;
  }

 +/// Determine whether the attributes we can overload on are identical
 for A and
 +/// B. Expects A and B to (otherwise) have the same type.
 +static bool hasSameOverloadableAttrs(const FunctionDecl *A,
 + const FunctionDecl *B) {
 +  SmallVector AEnableIfs;
 +  // Since this is an equality check, we can ignore that enable_if
 attrs show up
 +  // in reverse order.
 +  for (const auto *EIA : A->specific_attrs())
 +AEnableIfs.push_back(EIA);
 +
 +  SmallVector BEnableIfs;
 +  for (const auto *EIA : B->specific_attrs())
 +BEnableIfs.push_back(EIA);
 +
 +  // Two very common cases: either we have 0 enable_if attrs, or we
 have an
 +  // unequal number of enable_if attrs.
 +  if (AEnableIfs.empty() && BEnableIfs.empty())
 +return true;
 +
 +  if (AEnableIfs.size() != BEnableIfs.size())
 +return false;
 +
 +  llvm::FoldingSetNodeID Cand1ID, Cand2ID;
 +  for (unsigned I = 0, E = AEnableIfs.size(); I != E; ++I) {
 +Cand1ID.clear();
 +Cand2ID.clear();
 +
 +AEnableIfs[I]->getCond()->Profile(Cand1ID, A->getASTContext(),
 true);
 +BEnableIfs[I]->getCond()->Profile(Cand2ID, B->getASTContext(),
 true);
 +if (Cand1ID != Cand2ID)
 +  return false;
 +  }
 +
 +  // FIXME: This doesn't currently consider pass_object_size
 attributes, since
 +  // we aren't guaranteed that A and B have valid parameter lists yet.
 +  return true;
 +}
 +
  /// \brief Determine whether the two declarations 

Re: r295252 - [Modules] Consider enable_if attrs in isSameEntity.

[Richard Smith via cfe-commits]

On 15 February 2017 at 17:32, George Burgess IV  wrote:

> I remember that we wanted to pretend that pass_object_size isn't a part of
> the FunctionType during the review that added it, though.
>

I remember we wanted to not add extra fake "parameters" to the FunctionType
to model pass_object_size. I don't remember whether or why we wanted it to
not be part of the function type at all -- on reflection, it seems as much
a part of the type as, say, a calling convention (which it is, in some
sense).


> Do you think that would be better than serializing parameters before we
> serialize template info? AFAICT, we only do merging after we start reading
> the template info, so I can't immediately see why that wouldn't work.
>

I would be concerned about the possibility of that introducing dependency
cycles into the deserialization process. For instance, merging default
arguments for function parameters may require us to have already merged the
function itself into its redeclaration chain (we don't currently model that
quite correctly, so we probably won't hit it today).


> On Wed, Feb 15, 2017 at 4:55 PM, Richard Smith 
> wrote:
>
>> On 15 February 2017 at 14:43, George Burgess IV via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: gbiv
>>> Date: Wed Feb 15 16:43:27 2017
>>> New Revision: 295252
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=295252=rev
>>> Log:
>>> [Modules] Consider enable_if attrs in isSameEntity.
>>>
>>> Two functions that differ only in their enable_if attributes are
>>> considered overloads, so we should check for those when we're trying to
>>> figure out if two functions are mergeable.
>>>
>>> We need to do the same thing for pass_object_size, as well. Looks like
>>> that'll be a bit less trivial, since we sometimes do these merging
>>> checks before we have pass_object_size attributes available (see the
>>> merge checks in ASTDeclReader::VisitFunctionDecl that happen before we
>>> read parameters, and merge checks in calls to ReadDeclAs<>()).
>>>
>>
>> Perhaps the best way to tackle this would be to track the presence of
>> pass_object_size as part of the function type (in the ExtParameterInfo data
>> on the function type).
>>
>> Added:
>>> cfe/trunk/test/Modules/Inputs/overloadable-attrs/
>>> cfe/trunk/test/Modules/Inputs/overloadable-attrs/a.h
>>> cfe/trunk/test/Modules/Inputs/overloadable-attrs/module.modulemap
>>> cfe/trunk/test/Modules/overloadable-attrs.cpp
>>> Modified:
>>> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>>>
>>> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serializat
>>> ion/ASTReaderDecl.cpp?rev=295252=295251=295252=diff
>>> 
>>> ==
>>> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
>>> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Feb 15 16:43:27
>>> 2017
>>> @@ -2656,6 +2656,44 @@ static bool isSameTemplateParameterList(
>>>return true;
>>>  }
>>>
>>> +/// Determine whether the attributes we can overload on are identical
>>> for A and
>>> +/// B. Expects A and B to (otherwise) have the same type.
>>> +static bool hasSameOverloadableAttrs(const FunctionDecl *A,
>>> + const FunctionDecl *B) {
>>> +  SmallVector AEnableIfs;
>>> +  // Since this is an equality check, we can ignore that enable_if
>>> attrs show up
>>> +  // in reverse order.
>>> +  for (const auto *EIA : A->specific_attrs())
>>> +AEnableIfs.push_back(EIA);
>>> +
>>> +  SmallVector BEnableIfs;
>>> +  for (const auto *EIA : B->specific_attrs())
>>> +BEnableIfs.push_back(EIA);
>>> +
>>> +  // Two very common cases: either we have 0 enable_if attrs, or we
>>> have an
>>> +  // unequal number of enable_if attrs.
>>> +  if (AEnableIfs.empty() && BEnableIfs.empty())
>>> +return true;
>>> +
>>> +  if (AEnableIfs.size() != BEnableIfs.size())
>>> +return false;
>>> +
>>> +  llvm::FoldingSetNodeID Cand1ID, Cand2ID;
>>> +  for (unsigned I = 0, E = AEnableIfs.size(); I != E; ++I) {
>>> +Cand1ID.clear();
>>> +Cand2ID.clear();
>>> +
>>> +AEnableIfs[I]->getCond()->Profile(Cand1ID, A->getASTContext(),
>>> true);
>>> +BEnableIfs[I]->getCond()->Profile(Cand2ID, B->getASTContext(),
>>> true);
>>> +if (Cand1ID != Cand2ID)
>>> +  return false;
>>> +  }
>>> +
>>> +  // FIXME: This doesn't currently consider pass_object_size
>>> attributes, since
>>> +  // we aren't guaranteed that A and B have valid parameter lists yet.
>>> +  return true;
>>> +}
>>> +
>>>  /// \brief Determine whether the two declarations refer to the same
>>> entity.
>>>  static bool isSameEntity(NamedDecl *X, NamedDecl *Y) {
>>>assert(X->getDeclName() == Y->getDeclName() && "Declaration name
>>> mismatch!");
>>> @@ -2711,8 +2749,10 @@ static bool isSameEntity(NamedDecl *X, N
>>>  

Re: r295252 - [Modules] Consider enable_if attrs in isSameEntity.

[George Burgess IV via cfe-commits]

I remember that we wanted to pretend that pass_object_size isn't a part of
the FunctionType during the review that added it, though. Do you think that
would be better than serializing parameters before we serialize template
info? AFAICT, we only do merging after we start reading the template info,
so I can't immediately see why that wouldn't work.

On Wed, Feb 15, 2017 at 4:55 PM, Richard Smith 
wrote:

> On 15 February 2017 at 14:43, George Burgess IV via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: gbiv
>> Date: Wed Feb 15 16:43:27 2017
>> New Revision: 295252
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=295252=rev
>> Log:
>> [Modules] Consider enable_if attrs in isSameEntity.
>>
>> Two functions that differ only in their enable_if attributes are
>> considered overloads, so we should check for those when we're trying to
>> figure out if two functions are mergeable.
>>
>> We need to do the same thing for pass_object_size, as well. Looks like
>> that'll be a bit less trivial, since we sometimes do these merging
>> checks before we have pass_object_size attributes available (see the
>> merge checks in ASTDeclReader::VisitFunctionDecl that happen before we
>> read parameters, and merge checks in calls to ReadDeclAs<>()).
>>
>
> Perhaps the best way to tackle this would be to track the presence of
> pass_object_size as part of the function type (in the ExtParameterInfo data
> on the function type).
>
> Added:
>> cfe/trunk/test/Modules/Inputs/overloadable-attrs/
>> cfe/trunk/test/Modules/Inputs/overloadable-attrs/a.h
>> cfe/trunk/test/Modules/Inputs/overloadable-attrs/module.modulemap
>> cfe/trunk/test/Modules/overloadable-attrs.cpp
>> Modified:
>> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>>
>> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serializat
>> ion/ASTReaderDecl.cpp?rev=295252=295251=295252=diff
>> 
>> ==
>> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
>> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Feb 15 16:43:27
>> 2017
>> @@ -2656,6 +2656,44 @@ static bool isSameTemplateParameterList(
>>return true;
>>  }
>>
>> +/// Determine whether the attributes we can overload on are identical
>> for A and
>> +/// B. Expects A and B to (otherwise) have the same type.
>> +static bool hasSameOverloadableAttrs(const FunctionDecl *A,
>> + const FunctionDecl *B) {
>> +  SmallVector AEnableIfs;
>> +  // Since this is an equality check, we can ignore that enable_if attrs
>> show up
>> +  // in reverse order.
>> +  for (const auto *EIA : A->specific_attrs())
>> +AEnableIfs.push_back(EIA);
>> +
>> +  SmallVector BEnableIfs;
>> +  for (const auto *EIA : B->specific_attrs())
>> +BEnableIfs.push_back(EIA);
>> +
>> +  // Two very common cases: either we have 0 enable_if attrs, or we have
>> an
>> +  // unequal number of enable_if attrs.
>> +  if (AEnableIfs.empty() && BEnableIfs.empty())
>> +return true;
>> +
>> +  if (AEnableIfs.size() != BEnableIfs.size())
>> +return false;
>> +
>> +  llvm::FoldingSetNodeID Cand1ID, Cand2ID;
>> +  for (unsigned I = 0, E = AEnableIfs.size(); I != E; ++I) {
>> +Cand1ID.clear();
>> +Cand2ID.clear();
>> +
>> +AEnableIfs[I]->getCond()->Profile(Cand1ID, A->getASTContext(),
>> true);
>> +BEnableIfs[I]->getCond()->Profile(Cand2ID, B->getASTContext(),
>> true);
>> +if (Cand1ID != Cand2ID)
>> +  return false;
>> +  }
>> +
>> +  // FIXME: This doesn't currently consider pass_object_size attributes,
>> since
>> +  // we aren't guaranteed that A and B have valid parameter lists yet.
>> +  return true;
>> +}
>> +
>>  /// \brief Determine whether the two declarations refer to the same
>> entity.
>>  static bool isSameEntity(NamedDecl *X, NamedDecl *Y) {
>>assert(X->getDeclName() == Y->getDeclName() && "Declaration name
>> mismatch!");
>> @@ -2711,8 +2749,10 @@ static bool isSameEntity(NamedDecl *X, N
>>  CtorY->getInheritedConstructo
>> r().getConstructor()))
>>  return false;
>>  }
>> -return (FuncX->getLinkageInternal() == FuncY->getLinkageInternal())
>> &&
>> -  FuncX->getASTContext().hasSameType(FuncX->getType(),
>> FuncY->getType());
>> +return FuncX->getLinkageInternal() == FuncY->getLinkageInternal() &&
>> +   FuncX->getASTContext().hasSameType(FuncX->getType(),
>> +  FuncY->getType()) &&
>> +   hasSameOverloadableAttrs(FuncX, FuncY);
>>}
>>
>>// Variables with the same type and linkage match.
>>
>> Added: cfe/trunk/test/Modules/Inputs/overloadable-attrs/a.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/I
>> nputs/overloadable-attrs/a.h?rev=295252=auto
>> 
>> ==
>> 

Re: r295252 - [Modules] Consider enable_if attrs in isSameEntity.

[Richard Smith via cfe-commits]

On 15 February 2017 at 14:43, George Burgess IV via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: gbiv
> Date: Wed Feb 15 16:43:27 2017
> New Revision: 295252
>
> URL: http://llvm.org/viewvc/llvm-project?rev=295252=rev
> Log:
> [Modules] Consider enable_if attrs in isSameEntity.
>
> Two functions that differ only in their enable_if attributes are
> considered overloads, so we should check for those when we're trying to
> figure out if two functions are mergeable.
>
> We need to do the same thing for pass_object_size, as well. Looks like
> that'll be a bit less trivial, since we sometimes do these merging
> checks before we have pass_object_size attributes available (see the
> merge checks in ASTDeclReader::VisitFunctionDecl that happen before we
> read parameters, and merge checks in calls to ReadDeclAs<>()).
>

Perhaps the best way to tackle this would be to track the presence of
pass_object_size as part of the function type (in the ExtParameterInfo data
on the function type).

Added:
> cfe/trunk/test/Modules/Inputs/overloadable-attrs/
> cfe/trunk/test/Modules/Inputs/overloadable-attrs/a.h
> cfe/trunk/test/Modules/Inputs/overloadable-attrs/module.modulemap
> cfe/trunk/test/Modules/overloadable-attrs.cpp
> Modified:
> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>
> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/
> Serialization/ASTReaderDecl.cpp?rev=295252=295251=295252=diff
> 
> ==
> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Feb 15 16:43:27 2017
> @@ -2656,6 +2656,44 @@ static bool isSameTemplateParameterList(
>return true;
>  }
>
> +/// Determine whether the attributes we can overload on are identical for
> A and
> +/// B. Expects A and B to (otherwise) have the same type.
> +static bool hasSameOverloadableAttrs(const FunctionDecl *A,
> + const FunctionDecl *B) {
> +  SmallVector AEnableIfs;
> +  // Since this is an equality check, we can ignore that enable_if attrs
> show up
> +  // in reverse order.
> +  for (const auto *EIA : A->specific_attrs())
> +AEnableIfs.push_back(EIA);
> +
> +  SmallVector BEnableIfs;
> +  for (const auto *EIA : B->specific_attrs())
> +BEnableIfs.push_back(EIA);
> +
> +  // Two very common cases: either we have 0 enable_if attrs, or we have
> an
> +  // unequal number of enable_if attrs.
> +  if (AEnableIfs.empty() && BEnableIfs.empty())
> +return true;
> +
> +  if (AEnableIfs.size() != BEnableIfs.size())
> +return false;
> +
> +  llvm::FoldingSetNodeID Cand1ID, Cand2ID;
> +  for (unsigned I = 0, E = AEnableIfs.size(); I != E; ++I) {
> +Cand1ID.clear();
> +Cand2ID.clear();
> +
> +AEnableIfs[I]->getCond()->Profile(Cand1ID, A->getASTContext(), true);
> +BEnableIfs[I]->getCond()->Profile(Cand2ID, B->getASTContext(), true);
> +if (Cand1ID != Cand2ID)
> +  return false;
> +  }
> +
> +  // FIXME: This doesn't currently consider pass_object_size attributes,
> since
> +  // we aren't guaranteed that A and B have valid parameter lists yet.
> +  return true;
> +}
> +
>  /// \brief Determine whether the two declarations refer to the same
> entity.
>  static bool isSameEntity(NamedDecl *X, NamedDecl *Y) {
>assert(X->getDeclName() == Y->getDeclName() && "Declaration name
> mismatch!");
> @@ -2711,8 +2749,10 @@ static bool isSameEntity(NamedDecl *X, N
>  CtorY->getInheritedConstructor().
> getConstructor()))
>  return false;
>  }
> -return (FuncX->getLinkageInternal() == FuncY->getLinkageInternal()) &&
> -  FuncX->getASTContext().hasSameType(FuncX->getType(),
> FuncY->getType());
> +return FuncX->getLinkageInternal() == FuncY->getLinkageInternal() &&
> +   FuncX->getASTContext().hasSameType(FuncX->getType(),
> +  FuncY->getType()) &&
> +   hasSameOverloadableAttrs(FuncX, FuncY);
>}
>
>// Variables with the same type and linkage match.
>
> Added: cfe/trunk/test/Modules/Inputs/overloadable-attrs/a.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/
> Modules/Inputs/overloadable-attrs/a.h?rev=295252=auto
> 
> ==
> --- cfe/trunk/test/Modules/Inputs/overloadable-attrs/a.h (added)
> +++ cfe/trunk/test/Modules/Inputs/overloadable-attrs/a.h Wed Feb 15
> 16:43:27 2017
> @@ -0,0 +1,16 @@
> +namespace enable_if_attrs {
> +constexpr int fn1() __attribute__((enable_if(0, ""))) { return 0; }
> +constexpr int fn1() { return 1; }
> +
> +constexpr int fn2() { return 1; }
> +constexpr int fn2() __attribute__((enable_if(0, ""))) { return 0; }
> +
> +constexpr int fn3(int i) __attribute__((enable_if(!i, ""))) { return 0; }
> +constexpr int fn3(int i) __attribute__((enable_if(i,