Re: [PATCH] D17933: Set MaxAtomicInlineWidth properly for i386, i486, and x86-64 cpus without cmpxchg16b.

2016-04-03 Thread David Majnemer via cfe-commits 
majnemer added a subscriber: majnemer.


Comment at: lib/Basic/Targets.cpp:2565-2577
@@ -2569,1 +2564,15 @@
+
+  void setAtomic() {
+if (getTriple().getArch() == llvm::Triple::x86_64) {
+  if (HasCX16)
+MaxAtomicInlineWidth = 128;
+  else
+MaxAtomicInlineWidth = 64;
+} else if (CPU >= CK_i586)
+  MaxAtomicInlineWidth = 64;
+else if (CPU >= CK_i486)
+  MaxAtomicInlineWidth = 32;
+else
+  MaxAtomicInlineWidth = 0;
+  }
 };

Might be easier to read if we go from increasing strength:

```
void setAtomic() {
  MaxAtomicInlineWidth = 0;
  if (CPU >= CK_i486)
MaxAtomicInlineWidth = 32;
  if (CPU >= CK_i586)
MaxAtomicInlineWidth = 64;
  if (getTriple().getArch() == llvm::Triple::x86_64) {
MaxAtomicInlineWidth = 64;
if (HasCX16)
  MaxAtomicInlineWidth = 128;
  }
}
```

Either works for me.


http://reviews.llvm.org/D17933



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[scan-build] Fix reported logic error and api bugs

2016-04-03 Thread Apelete Seketeli via cfe-commits 
Hello,

Here are a few changes that try to address some logic error bugs
reported by running scan-build over the Clang code base.

I have no background in compiler technology and I may have been trying
to fix false positives; here is a quick overview of what scan-build
reported, let me know if the fixes are relevant:

Bug Group: Logic error
Bug Type: Called C++ object pointer is null
Files: lib/Driver/Tools.cpp

Bug Group: Logic error
Bug Type: Called C++ object pointer is null
Files: lib/Sema/SemaInit.cpp

Bug Group: API
Bug Type: Argument with 'nonnull' attribute passed null
Files: lib/AST/NestedNameSpecifier.cpp

Cheers.
-- 
Apelete
diff --git a/lib/AST/NestedNameSpecifier.cpp b/lib/AST/NestedNameSpecifier.cpp
index ede3862..a3b1076 100644
--- a/lib/AST/NestedNameSpecifier.cpp
+++ b/lib/AST/NestedNameSpecifier.cpp
@@ -456,9 +456,11 @@ namespace {
   Buffer = NewBuffer;
   BufferCapacity = NewCapacity;
 }
-
-memcpy(Buffer + BufferSize, Start, End - Start);
-BufferSize += End-Start;
+
+if (Buffer) {
+  memcpy(Buffer + BufferSize, Start, End - Start);
+  BufferSize += End-Start;
+}
   }
   
   /// \brief Save a source location to the given buffer.
diff --git a/lib/Driver/Tools.cpp b/lib/Driver/Tools.cpp
index 824ee72..2ac5416 100644
--- a/lib/Driver/Tools.cpp
+++ b/lib/Driver/Tools.cpp
@@ -2334,7 +2334,7 @@ static void getAArch64TargetFeatures(const Driver &D, const ArgList &Args,
 success = getAArch64MicroArchFeaturesFromMcpu(D, getAArch64TargetCPU(Args),
   Args, Features);
 
-  if (!success)
+  if (!success && A)
 D.Diag(diag::err_drv_clang_unsupported) << A->getAsString(Args);
 
   if (Args.getLastArg(options::OPT_mgeneral_regs_only)) {
diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp
index bd8522d..dc4e967 100644
--- a/lib/Sema/SemaInit.cpp
+++ b/lib/Sema/SemaInit.cpp
@@ -4860,6 +4860,8 @@ static bool tryObjCWritebackConversion(Sema &S,
InitializationSequence &Sequence,
const InitializedEntity &Entity,
Expr *Initializer) {
+  assert(Initializer && "Initializer needs to be not NULL");
+
   bool ArrayDecay = false;
   QualType ArgType = Initializer->getType();
   QualType ArgPointee;
@@ -5235,11 +5237,11 @@ void InitializationSequence::InitializeFrom(Sema &S,
 DeclAccessPair dap;
 if (isLibstdcxxPointerReturnFalseHack(S, Entity, Initializer)) {
   AddZeroInitializationStep(Entity.getType());
-} else if (Initializer->getType() == Context.OverloadTy &&
+} else if (Initializer && Initializer->getType() == Context.OverloadTy &&
!S.ResolveAddressOfOverloadedFunction(Initializer, DestType,
  false, dap))
   SetFailed(InitializationSequence::FK_AddressOfOverloadFailed);
-else if (Initializer->getType()->isFunctionType() &&
+else if (Initializer && Initializer->getType()->isFunctionType() &&
  isExprAnUnaddressableFunction(S, Initializer))
   SetFailed(InitializationSequence::FK_AddressOfUnaddressableFunction);
 else
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r259734 - Fix predefine for __NSConstantString struct type

2016-04-03 Thread Vassil Vassilev via cfe-commits 

On 04/02/16 01:55, Ben Langmuir via cfe-commits wrote:

Author: benlangmuir
Date: Wed Feb  3 18:55:24 2016
New Revision: 259734

URL: http://llvm.org/viewvc/llvm-project?rev=259734&view=rev
Log:
Fix predefine for __NSConstantString struct type

Per review feedback the name was wrong and it can be used outside
Objective-C.

Unfortunately, making the internal struct visible broke some ASTMatchers
tests that assumed that the first record decl would be from user code,
rather than a builtin type.  I'm worried that this will also affect
users' code.  So this patch adds a typedef to wrap the internal struct
and only makes the typedef visible to namelookup.  This is sufficient to
allow the ASTReader to merge the decls we need without making the struct
itself visible.

rdar://problem/24425801

Modified:
 cfe/trunk/include/clang/AST/ASTContext.h
 cfe/trunk/include/clang/Serialization/ASTBitCodes.h
 cfe/trunk/lib/AST/ASTContext.cpp
 cfe/trunk/lib/Sema/Sema.cpp
 cfe/trunk/lib/Serialization/ASTReader.cpp
 cfe/trunk/lib/Serialization/ASTWriter.cpp
 cfe/trunk/test/CodeGenObjC/2010-02-01-utf16-with-null.m
 cfe/trunk/test/CodeGenObjC/arc-no-arc-exceptions.m
 cfe/trunk/test/CodeGenObjC/tentative-cfconstantstring.m
 cfe/trunk/test/Modules/Inputs/builtin.h
 cfe/trunk/test/Modules/builtins.m

Modified: cfe/trunk/include/clang/AST/ASTContext.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=259734&r1=259733&r2=259734&view=diff
==
--- cfe/trunk/include/clang/AST/ASTContext.h (original)
+++ cfe/trunk/include/clang/AST/ASTContext.h Wed Feb  3 18:55:24 2016
@@ -253,8 +253,9 @@ class ASTContext : public RefCountedBase
mutable IdentifierInfo *MakeIntegerSeqName = nullptr;
  
QualType ObjCConstantStringType;

-  mutable RecordDecl *CFConstantStringTypeDecl;
-
+  mutable RecordDecl *CFConstantStringTagDecl;
+  mutable TypedefDecl *CFConstantStringTypeDecl;
+
mutable QualType ObjCSuperType;

QualType ObjCNSStringType;

@@ -1381,11 +1382,12 @@ public:
/// if it hasn't yet been built.
QualType getRawCFConstantStringType() const {
  if (CFConstantStringTypeDecl)
-  return getTagDeclType(CFConstantStringTypeDecl);
+  return getTypedefType(CFConstantStringTypeDecl);
  return QualType();
}
void setCFConstantStringType(QualType T);
-  TagDecl *getCFConstantStringDecl() const;
+  TypedefDecl *getCFConstantStringDecl() const;
+  RecordDecl *getCFConstantStringTagDecl() const;
  
// This setter/getter represents the ObjC type for an NSConstantString.

void setObjCConstantStringInterface(ObjCInterfaceDecl *Decl);

Modified: cfe/trunk/include/clang/Serialization/ASTBitCodes.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTBitCodes.h?rev=259734&r1=259733&r2=259734&view=diff
==
--- cfe/trunk/include/clang/Serialization/ASTBitCodes.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTBitCodes.h Wed Feb  3 18:55:24 2016
@@ -988,15 +988,18 @@ namespace clang {
/// \brief The internal '__make_integer_seq' template.
PREDEF_DECL_MAKE_INTEGER_SEQ_ID = 13,
  
-  /// \brief The internal '__NSConstantString' type.

+  /// \brief The internal '__NSConstantString' typedef.
PREDEF_DECL_CF_CONSTANT_STRING_ID = 14,
+
+  /// \brief The internal '__NSConstantString' tag type.
+  PREDEF_DECL_CF_CONSTANT_STRING_TAG_ID = 15,
  };
  
  /// \brief The number of declaration IDs that are predefined.

  ///
  /// For more information about predefined declarations, see the
  /// \c PredefinedDeclIDs type and the PREDEF_DECL_*_ID constants.
-const unsigned int NUM_PREDEF_DECL_IDS = 15;
+const unsigned int NUM_PREDEF_DECL_IDS = 16;
  
  /// \brief Record code for a list of local redeclarations of a declaration.

  const unsigned int LOCAL_REDECLARATIONS = 50;

Modified: cfe/trunk/lib/AST/ASTContext.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=259734&r1=259733&r2=259734&view=diff
==
--- cfe/trunk/lib/AST/ASTContext.cpp (original)
+++ cfe/trunk/lib/AST/ASTContext.cpp Wed Feb  3 18:55:24 2016
@@ -738,12 +738,13 @@ ASTContext::ASTContext(LangOptions &LOpt
BuiltinVaListDecl(nullptr), BuiltinMSVaListDecl(nullptr),
ObjCIdDecl(nullptr), ObjCSelDecl(nullptr), ObjCClassDecl(nullptr),
ObjCProtocolClassDecl(nullptr), BOOLDecl(nullptr),
-  CFConstantStringTypeDecl(nullptr), ObjCInstanceTypeDecl(nullptr),
-  FILEDecl(nullptr), jmp_bufDecl(nullptr), sigjmp_bufDecl(nullptr),
-  ucontext_tDecl(nullptr), BlockDescriptorType(nullptr),
-  BlockDescriptorExtendedType(nullptr), cudaConfigureCallDecl(nullptr),
-  FirstLocalImport(), LastLoc

Re: r259734 - Fix predefine for __NSConstantString struct type

2016-04-03 Thread Yaron Keren via cfe-commits 
+1, if possible. There is a if (getLangOpts().ObjC1) block just before this.

2016-04-03 14:21 GMT+03:00 Vassil Vassilev via cfe-commits <
cfe-commits@lists.llvm.org>:

> On 04/02/16 01:55, Ben Langmuir via cfe-commits wrote:
>
>> Author: benlangmuir
>> Date: Wed Feb  3 18:55:24 2016
>> New Revision: 259734
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=259734&view=rev
>> Log:
>> Fix predefine for __NSConstantString struct type
>>
>> Per review feedback the name was wrong and it can be used outside
>> Objective-C.
>>
>> Unfortunately, making the internal struct visible broke some ASTMatchers
>> tests that assumed that the first record decl would be from user code,
>> rather than a builtin type.  I'm worried that this will also affect
>> users' code.  So this patch adds a typedef to wrap the internal struct
>> and only makes the typedef visible to namelookup.  This is sufficient to
>> allow the ASTReader to merge the decls we need without making the struct
>> itself visible.
>>
>> rdar://problem/24425801
>>
>> Modified:
>>  cfe/trunk/include/clang/AST/ASTContext.h
>>  cfe/trunk/include/clang/Serialization/ASTBitCodes.h
>>  cfe/trunk/lib/AST/ASTContext.cpp
>>  cfe/trunk/lib/Sema/Sema.cpp
>>  cfe/trunk/lib/Serialization/ASTReader.cpp
>>  cfe/trunk/lib/Serialization/ASTWriter.cpp
>>  cfe/trunk/test/CodeGenObjC/2010-02-01-utf16-with-null.m
>>  cfe/trunk/test/CodeGenObjC/arc-no-arc-exceptions.m
>>  cfe/trunk/test/CodeGenObjC/tentative-cfconstantstring.m
>>  cfe/trunk/test/Modules/Inputs/builtin.h
>>  cfe/trunk/test/Modules/builtins.m
>>
>> Modified: cfe/trunk/include/clang/AST/ASTContext.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=259734&r1=259733&r2=259734&view=diff
>>
>> ==
>> --- cfe/trunk/include/clang/AST/ASTContext.h (original)
>> +++ cfe/trunk/include/clang/AST/ASTContext.h Wed Feb  3 18:55:24 2016
>> @@ -253,8 +253,9 @@ class ASTContext : public RefCountedBase
>> mutable IdentifierInfo *MakeIntegerSeqName = nullptr;
>>   QualType ObjCConstantStringType;
>> -  mutable RecordDecl *CFConstantStringTypeDecl;
>> -
>> +  mutable RecordDecl *CFConstantStringTagDecl;
>> +  mutable TypedefDecl *CFConstantStringTypeDecl;
>> +
>> mutable QualType ObjCSuperType;
>> QualType ObjCNSStringType;
>> @@ -1381,11 +1382,12 @@ public:
>> /// if it hasn't yet been built.
>> QualType getRawCFConstantStringType() const {
>>   if (CFConstantStringTypeDecl)
>> -  return getTagDeclType(CFConstantStringTypeDecl);
>> +  return getTypedefType(CFConstantStringTypeDecl);
>>   return QualType();
>> }
>> void setCFConstantStringType(QualType T);
>> -  TagDecl *getCFConstantStringDecl() const;
>> +  TypedefDecl *getCFConstantStringDecl() const;
>> +  RecordDecl *getCFConstantStringTagDecl() const;
>>   // This setter/getter represents the ObjC type for an
>> NSConstantString.
>> void setObjCConstantStringInterface(ObjCInterfaceDecl *Decl);
>>
>> Modified: cfe/trunk/include/clang/Serialization/ASTBitCodes.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTBitCodes.h?rev=259734&r1=259733&r2=259734&view=diff
>>
>> ==
>> --- cfe/trunk/include/clang/Serialization/ASTBitCodes.h (original)
>> +++ cfe/trunk/include/clang/Serialization/ASTBitCodes.h Wed Feb  3
>> 18:55:24 2016
>> @@ -988,15 +988,18 @@ namespace clang {
>> /// \brief The internal '__make_integer_seq' template.
>> PREDEF_DECL_MAKE_INTEGER_SEQ_ID = 13,
>>   -  /// \brief The internal '__NSConstantString' type.
>> +  /// \brief The internal '__NSConstantString' typedef.
>> PREDEF_DECL_CF_CONSTANT_STRING_ID = 14,
>> +
>> +  /// \brief The internal '__NSConstantString' tag type.
>> +  PREDEF_DECL_CF_CONSTANT_STRING_TAG_ID = 15,
>>   };
>> /// \brief The number of declaration IDs that are predefined.
>>   ///
>>   /// For more information about predefined declarations, see the
>>   /// \c PredefinedDeclIDs type and the PREDEF_DECL_*_ID constants.
>> -const unsigned int NUM_PREDEF_DECL_IDS = 15;
>> +const unsigned int NUM_PREDEF_DECL_IDS = 16;
>> /// \brief Record code for a list of local redeclarations of a
>> declaration.
>>   const unsigned int LOCAL_REDECLARATIONS = 50;
>>
>> Modified: cfe/trunk/lib/AST/ASTContext.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=259734&r1=259733&r2=259734&view=diff
>>
>> ==
>> --- cfe/trunk/lib/AST/ASTContext.cpp (original)
>> +++ cfe/trunk/lib/AST/ASTContext.cpp Wed Feb  3 18:55:24 2016
>> @@ -738,12 +738,13 @@ ASTContext::ASTContext(LangOptions &LOpt
>> BuiltinVaListDecl(nullptr), BuiltinMSVaListDecl(nul

Re: r259734 - Fix predefine for __NSConstantString struct type

2016-04-03 Thread Ben Langmuir via cfe-commits 
This type and the builtin it supports needs to work in C/C++ as well, not just 
Objective-C.   Are you running into a problem here, or is it just “weird” to 
see it in the AST?

> On Apr 3, 2016, at 5:21 AM, Yaron Keren  wrote:
> 
> +1, if possible. There is a if (getLangOpts().ObjC1) block just before this.
> 
> 2016-04-03 14:21 GMT+03:00 Vassil Vassilev via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>>:
> On 04/02/16 01:55, Ben Langmuir via cfe-commits wrote:
> Author: benlangmuir
> Date: Wed Feb  3 18:55:24 2016
> New Revision: 259734
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=259734&view=rev 
> 
> Log:
> Fix predefine for __NSConstantString struct type
> 
> Per review feedback the name was wrong and it can be used outside
> Objective-C.
> 
> Unfortunately, making the internal struct visible broke some ASTMatchers
> tests that assumed that the first record decl would be from user code,
> rather than a builtin type.  I'm worried that this will also affect
> users' code.  So this patch adds a typedef to wrap the internal struct
> and only makes the typedef visible to namelookup.  This is sufficient to
> allow the ASTReader to merge the decls we need without making the struct
> itself visible.
> 
> rdar://problem/24425801
> 
> Modified:
>  cfe/trunk/include/clang/AST/ASTContext.h
>  cfe/trunk/include/clang/Serialization/ASTBitCodes.h
>  cfe/trunk/lib/AST/ASTContext.cpp
>  cfe/trunk/lib/Sema/Sema.cpp
>  cfe/trunk/lib/Serialization/ASTReader.cpp
>  cfe/trunk/lib/Serialization/ASTWriter.cpp
>  cfe/trunk/test/CodeGenObjC/2010-02-01-utf16-with-null.m
>  cfe/trunk/test/CodeGenObjC/arc-no-arc-exceptions.m
>  cfe/trunk/test/CodeGenObjC/tentative-cfconstantstring.m
>  cfe/trunk/test/Modules/Inputs/builtin.h
>  cfe/trunk/test/Modules/builtins.m
> 
> Modified: cfe/trunk/include/clang/AST/ASTContext.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=259734&r1=259733&r2=259734&view=diff
>  
> 
> ==
> --- cfe/trunk/include/clang/AST/ASTContext.h (original)
> +++ cfe/trunk/include/clang/AST/ASTContext.h Wed Feb  3 18:55:24 2016
> @@ -253,8 +253,9 @@ class ASTContext : public RefCountedBase
> mutable IdentifierInfo *MakeIntegerSeqName = nullptr;
>   QualType ObjCConstantStringType;
> -  mutable RecordDecl *CFConstantStringTypeDecl;
> -
> +  mutable RecordDecl *CFConstantStringTagDecl;
> +  mutable TypedefDecl *CFConstantStringTypeDecl;
> +
> mutable QualType ObjCSuperType;
> QualType ObjCNSStringType;
> @@ -1381,11 +1382,12 @@ public:
> /// if it hasn't yet been built.
> QualType getRawCFConstantStringType() const {
>   if (CFConstantStringTypeDecl)
> -  return getTagDeclType(CFConstantStringTypeDecl);
> +  return getTypedefType(CFConstantStringTypeDecl);
>   return QualType();
> }
> void setCFConstantStringType(QualType T);
> -  TagDecl *getCFConstantStringDecl() const;
> +  TypedefDecl *getCFConstantStringDecl() const;
> +  RecordDecl *getCFConstantStringTagDecl() const;
>   // This setter/getter represents the ObjC type for an NSConstantString.
> void setObjCConstantStringInterface(ObjCInterfaceDecl *Decl);
> 
> Modified: cfe/trunk/include/clang/Serialization/ASTBitCodes.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTBitCodes.h?rev=259734&r1=259733&r2=259734&view=diff
>  
> 
> ==
> --- cfe/trunk/include/clang/Serialization/ASTBitCodes.h (original)
> +++ cfe/trunk/include/clang/Serialization/ASTBitCodes.h Wed Feb  3 18:55:24 
> 2016
> @@ -988,15 +988,18 @@ namespace clang {
> /// \brief The internal '__make_integer_seq' template.
> PREDEF_DECL_MAKE_INTEGER_SEQ_ID = 13,
>   -  /// \brief The internal '__NSConstantString' type.
> +  /// \brief The internal '__NSConstantString' typedef.
> PREDEF_DECL_CF_CONSTANT_STRING_ID = 14,
> +
> +  /// \brief The internal '__NSConstantString' tag type.
> +  PREDEF_DECL_CF_CONSTANT_STRING_TAG_ID = 15,
>   };
> /// \brief The number of declaration IDs that are predefined.
>   ///
>   /// For more information about predefined declarations, see the
>   /// \c PredefinedDeclIDs type and the PREDEF_DECL_*_ID constants.
> -const unsigned int NUM_PREDEF_DECL_IDS = 15;
> +const unsigned int NUM_PREDEF_DECL_IDS = 16;
> /// \brief Record code for a list of local redeclarations of a 
> declaration.
>   const unsigned int LOCAL_REDECLARATIONS = 50;
> 
> Modified: cfe/tr

Re: r259734 - Fix predefine for __NSConstantString struct type

2016-04-03 Thread Yaron Keren via cfe-commits 
I look at many AST dumps for small reduced code so it's mostly weird to see
this in the AST (and takes a bit of screen real estate) as it's not
something that originated in the code.
By now I'm used to ignoring it but would expect NSConstantString would
baffle new clang programmers. It would be nice if we won't see it...
How is NSConstantString used outside ObjectiveC ?




2016-04-03 20:39 GMT+03:00 Ben Langmuir :

> This type and the builtin it supports needs to work in C/C++ as well, not
> just Objective-C.   Are you running into a problem here, or is it just
> “weird” to see it in the AST?
>
> On Apr 3, 2016, at 5:21 AM, Yaron Keren  wrote:
>
> +1, if possible. There is a if (getLangOpts().ObjC1) block just before
> this.
>
> 2016-04-03 14:21 GMT+03:00 Vassil Vassilev via cfe-commits <
> cfe-commits@lists.llvm.org>:
>
>> On 04/02/16 01:55, Ben Langmuir via cfe-commits wrote:
>>
>>> Author: benlangmuir
>>> Date: Wed Feb  3 18:55:24 2016
>>> New Revision: 259734
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=259734&view=rev
>>> Log:
>>> Fix predefine for __NSConstantString struct type
>>>
>>> Per review feedback the name was wrong and it can be used outside
>>> Objective-C.
>>>
>>> Unfortunately, making the internal struct visible broke some ASTMatchers
>>> tests that assumed that the first record decl would be from user code,
>>> rather than a builtin type.  I'm worried that this will also affect
>>> users' code.  So this patch adds a typedef to wrap the internal struct
>>> and only makes the typedef visible to namelookup.  This is sufficient to
>>> allow the ASTReader to merge the decls we need without making the struct
>>> itself visible.
>>>
>>> rdar://problem/24425801
>>>
>>> Modified:
>>>  cfe/trunk/include/clang/AST/ASTContext.h
>>>  cfe/trunk/include/clang/Serialization/ASTBitCodes.h
>>>  cfe/trunk/lib/AST/ASTContext.cpp
>>>  cfe/trunk/lib/Sema/Sema.cpp
>>>  cfe/trunk/lib/Serialization/ASTReader.cpp
>>>  cfe/trunk/lib/Serialization/ASTWriter.cpp
>>>  cfe/trunk/test/CodeGenObjC/2010-02-01-utf16-with-null.m
>>>  cfe/trunk/test/CodeGenObjC/arc-no-arc-exceptions.m
>>>  cfe/trunk/test/CodeGenObjC/tentative-cfconstantstring.m
>>>  cfe/trunk/test/Modules/Inputs/builtin.h
>>>  cfe/trunk/test/Modules/builtins.m
>>>
>>> Modified: cfe/trunk/include/clang/AST/ASTContext.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=259734&r1=259733&r2=259734&view=diff
>>>
>>> ==
>>> --- cfe/trunk/include/clang/AST/ASTContext.h (original)
>>> +++ cfe/trunk/include/clang/AST/ASTContext.h Wed Feb  3 18:55:24 2016
>>> @@ -253,8 +253,9 @@ class ASTContext : public RefCountedBase
>>> mutable IdentifierInfo *MakeIntegerSeqName = nullptr;
>>>   QualType ObjCConstantStringType;
>>> -  mutable RecordDecl *CFConstantStringTypeDecl;
>>> -
>>> +  mutable RecordDecl *CFConstantStringTagDecl;
>>> +  mutable TypedefDecl *CFConstantStringTypeDecl;
>>> +
>>> mutable QualType ObjCSuperType;
>>> QualType ObjCNSStringType;
>>> @@ -1381,11 +1382,12 @@ public:
>>> /// if it hasn't yet been built.
>>> QualType getRawCFConstantStringType() const {
>>>   if (CFConstantStringTypeDecl)
>>> -  return getTagDeclType(CFConstantStringTypeDecl);
>>> +  return getTypedefType(CFConstantStringTypeDecl);
>>>   return QualType();
>>> }
>>> void setCFConstantStringType(QualType T);
>>> -  TagDecl *getCFConstantStringDecl() const;
>>> +  TypedefDecl *getCFConstantStringDecl() const;
>>> +  RecordDecl *getCFConstantStringTagDecl() const;
>>>   // This setter/getter represents the ObjC type for an
>>> NSConstantString.
>>> void setObjCConstantStringInterface(ObjCInterfaceDecl *Decl);
>>>
>>> Modified: cfe/trunk/include/clang/Serialization/ASTBitCodes.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTBitCodes.h?rev=259734&r1=259733&r2=259734&view=diff
>>>
>>> ==
>>> --- cfe/trunk/include/clang/Serialization/ASTBitCodes.h (original)
>>> +++ cfe/trunk/include/clang/Serialization/ASTBitCodes.h Wed Feb  3
>>> 18:55:24 2016
>>> @@ -988,15 +988,18 @@ namespace clang {
>>> /// \brief The internal '__make_integer_seq' template.
>>> PREDEF_DECL_MAKE_INTEGER_SEQ_ID = 13,
>>>   -  /// \brief The internal '__NSConstantString' type.
>>> +  /// \brief The internal '__NSConstantString' typedef.
>>> PREDEF_DECL_CF_CONSTANT_STRING_ID = 14,
>>> +
>>> +  /// \brief The internal '__NSConstantString' tag type.
>>> +  PREDEF_DECL_CF_CONSTANT_STRING_TAG_ID = 15,
>>>   };
>>> /// \brief The number of declaration IDs that are predefined.
>>>   ///
>>>   /// For more information about predefined declarations, see the
>>>   /// \c PredefinedDeclIDs type and the PREDEF_DECL_*_ID const

[PATCH] D18747: [Support] Fix an invalid character escaping in string literal (unittest).

2016-04-03 Thread Etienne Bergeron via cfe-commits 
etienneb created this revision.
etienneb added reviewers: rnk, alexfh.
etienneb added a subscriber: cfe-commits.

A character within a string literal is not escaped correctly.
In this case, there is no semantic change because the invalid character turn 
out to be NUL anyway.

note: "\0x12" is equivalent to {0, 'x', '1', '2'} and not { 12 }.

This issue was found by clang-tidy.

http://reviews.llvm.org/D18747

Files:
  unittests/Support/Path.cpp

Index: unittests/Support/Path.cpp
===
--- unittests/Support/Path.cpp
+++ unittests/Support/Path.cpp
@@ -725,7 +725,7 @@
 const char coff_import_library[] = "\x00\x00\xff\xff";
 const char elf_relocatable[] = { 0x7f, 'E', 'L', 'F', 1, 2, 1, 0, 0,
  0,0,   0,   0,   0, 0, 0, 0, 1 };
-const char macho_universal_binary[] = "\xca\xfe\xba\xbe...\0x00";
+const char macho_universal_binary[] = "\xca\xfe\xba\xbe...\x00";
 const char macho_object[] =
 "\xfe\xed\xfa\xce\x00\x00\x00\x01";
 const char macho_executable[] =


Index: unittests/Support/Path.cpp
===
--- unittests/Support/Path.cpp
+++ unittests/Support/Path.cpp
@@ -725,7 +725,7 @@
 const char coff_import_library[] = "\x00\x00\xff\xff";
 const char elf_relocatable[] = { 0x7f, 'E', 'L', 'F', 1, 2, 1, 0, 0,
  0,0,   0,   0,   0, 0, 0, 0, 1 };
-const char macho_universal_binary[] = "\xca\xfe\xba\xbe...\0x00";
+const char macho_universal_binary[] = "\xca\xfe\xba\xbe...\x00";
 const char macho_object[] =
 "\xfe\xed\xfa\xce\x00\x00\x00\x01";
 const char macho_executable[] =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15031: CFG: Add CFGElement for automatic variables that leave the scope

2016-04-03 Thread Matthias Gehre via cfe-commits 
mgehre added a comment.

Thank you very much for your review. I'm sorry that I'm currently quite busy, 
but I will find some time to answer at the end of this week.


http://reviews.llvm.org/D15031



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18745: [clang-tidy] Adds misc-use-bool-literals check.

2016-04-03 Thread Eugene Zelenko via cfe-commits 
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.

Isn't readability-implicit-bool-cast¶ should catch such issues? If not, I think 
will be good idea to improve that check instead of introducing new one.


http://reviews.llvm.org/D18745



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

2016-04-03 Thread Andy Gibbs via cfe-commits 
AndyG updated this revision to Diff 52513.
AndyG added a comment.

Ok, I've removed support for nested parentheses.  Can this go through now?

Thanks.


http://reviews.llvm.org/D17149

Files:
  include/clang/Basic/DiagnosticLexKinds.td
  lib/Lex/PPMacroExpansion.cpp
  test/Preprocessor/feature_tests.c
  test/Preprocessor/invalid-__has_warning1.c
  test/Preprocessor/invalid-__has_warning2.c
  test/Preprocessor/warning_tests.c

Index: test/Preprocessor/warning_tests.c
===
--- test/Preprocessor/warning_tests.c
+++ test/Preprocessor/warning_tests.c
@@ -12,7 +12,7 @@
 #endif
 
 // expected-error@+2 {{expected string literal in '__has_warning'}}
-// expected-error@+1 {{expected value in expression}}
+// expected-error@+1 {{missing ')'}} expected-note@+1 {{match}}
 #if __has_warning(-Wfoo)
 #endif
 
@@ -22,8 +22,7 @@
 #warning Not a valid warning flag
 #endif
 
-// expected-error@+2 {{builtin warning check macro requires a parenthesized string}}
-// expected-error@+1 {{invalid token}}
+// expected-error@+1 {{missing '(' after '__has_warning'}}
 #if __has_warning "not valid"
 #endif
 
@@ -33,7 +32,7 @@
 
 #define MY_ALIAS "-Wparentheses"
 
-// expected-error@+1 2{{expected}}
+// expected-error@+1 {{expected}}
 #if __has_warning(MY_ALIAS)
 #error Alias expansion not allowed
 #endif
Index: test/Preprocessor/invalid-__has_warning2.c
===
--- test/Preprocessor/invalid-__has_warning2.c
+++ test/Preprocessor/invalid-__has_warning2.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -verify %s
 
 // These must be the last lines in this test.
-// expected-error@+1{{expected string literal}} expected-error@+1{{expected}}
+// expected-error@+1{{too few arguments}}
 int i = __has_warning();
Index: test/Preprocessor/invalid-__has_warning1.c
===
--- test/Preprocessor/invalid-__has_warning1.c
+++ test/Preprocessor/invalid-__has_warning1.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -verify %s
 
 // These must be the last lines in this test.
-// expected-error@+1{{expected string literal}} expected-error@+1 2{{expected}}
+// expected-error@+1{{unterminated}} expected-error@+1 2{{expected}}
 int i = __has_warning(
Index: test/Preprocessor/feature_tests.c
===
--- test/Preprocessor/feature_tests.c
+++ test/Preprocessor/feature_tests.c
@@ -55,8 +55,50 @@
 #endif
 
 #ifdef VERIFY
-// expected-error@+2 {{builtin feature check macro requires a parenthesized identifier}}
-// expected-error@+1 {{expected value in expression}}
+// expected-error@+1 {{builtin feature check macro requires a parenthesized identifier}}
 #if __has_feature('x')
 #endif
+
+// The following are not identifiers:
+_Static_assert(!__is_identifier("string"), "oops");
+_Static_assert(!__is_identifier('c'), "oops");
+_Static_assert(!__is_identifier(123), "oops");
+_Static_assert(!__is_identifier(int), "oops");
+
+// The following are:
+_Static_assert(__is_identifier(abc /* comment */), "oops");
+_Static_assert(__is_identifier /* comment */ (xyz), "oops");
+
+// expected-error@+1 {{too few arguments}}
+#if __is_identifier()
+#endif
+
+// expected-error@+1 {{too many arguments}}
+#if __is_identifier(,())
+#endif
+
+// expected-error@+1 {{missing ')' after 'abc'}} 
+#if __is_identifier(abc xyz) // expected-note {{to match this '('}}
+#endif
+
+// expected-error@+1 {{missing ')' after 'abc'}} 
+#if __is_identifier(abc())   // expected-note {{to match this '('}}
+#endif
+
+// expected-error@+1 {{missing ')' after 'abc'}} 
+#if __is_identifier(abc.)// expected-note {{to match this '('}}
+#endif
+
+// expected-error@+1 {{'(' not expected after '('}} 
+#if __is_identifier((abc))
+#endif
+
+// expected-error@+1 {{missing '(' after '__is_identifier'}} expected-error@+1 {{expected value}}
+#if __is_identifier
+#endif
+
+// expected-error@+1 {{unterminated}} expected-error@+1 {{expected value}}
+#if __is_identifier(
+#endif
+
 #endif
Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -1053,9 +1053,8 @@
 
 /// HasFeature - Return true if we recognize and implement the feature
 /// specified by the identifier as a standard language feature.
-static bool HasFeature(const Preprocessor &PP, const IdentifierInfo *II) {
+static bool HasFeature(const Preprocessor &PP, StringRef Feature) {
   const LangOptions &LangOpts = PP.getLangOpts();
-  StringRef Feature = II->getName();
 
   // Normalize the feature name, __foo__ becomes foo.
   if (Feature.startswith("__") && Feature.endswith("__") && Feature.size() >= 4)
@@ -1229,8 +1228,8 @@
 /// HasExtension - Return true if we recognize and implement the feature
 /// specified by the identifier, either as an extension or a standard language
 /// feature.
-static bool HasExtension(const Prepro

[PATCH] D18748: [AMDGPU] Add debugger related target options

2016-04-03 Thread Konstantin Zhuravlyov via cfe-commits 
kzhuravl created this revision.
kzhuravl added a reviewer: arsenm.
kzhuravl added subscribers: cfe-commits, bpurnomo.
Herald added a reviewer: tstellarAMD.

-mamdgpu-debugger-abi=: sets all needed debugger related target 
features for given debugger abi 
-mamdgpu-debugger-insert-nops: sets +amdgpu-debugger-insert-nops target feature
-mamdgpu-debugger-reserve-trap-regs: sets +amdgpu-debugger-reserve-trap-regs 
target feature

http://reviews.llvm.org/D18748

Files:
  include/clang/Driver/Options.td
  lib/Driver/Tools.cpp
  test/Driver/amdgpu-features.c

Index: test/Driver/amdgpu-features.c
===
--- test/Driver/amdgpu-features.c
+++ test/Driver/amdgpu-features.c
@@ -0,0 +1,21 @@
+// Check handling of AMDGPU target features.
+//
+// -mamdgpu-debugger-abi=0.0
+// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri 
-mamdgpu-debugger-abi=0.0 %s -o 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-0-0 %s
+// CHECK-MAMDGPU-DEBUGGER-ABI-0-0: the clang compiler does not support 
'-mamdgpu-debugger-abi=0.0'
+//
+// -mamdgpu-debugger-abi=1.0
+// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri 
-mamdgpu-debugger-abi=1.0 %s -o 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-1-0 %s
+// CHECK-MAMDGPU-DEBUGGER-ABI-1-0: "-target-feature" 
"+amdgpu-debugger-insert-nops" "-target-feature" 
"+amdgpu-debugger-reserve-trap-regs"
+//
+// -mamdgpu-debugger-insert-nops
+// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri 
-mamdgpu-debugger-insert-nops %s -o 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-INSERT-NOPS %s
+// CHECK-MAMDGPU-DEBUGGER-INSERT-NOPS: "-target-feature" 
"+amdgpu-debugger-insert-nops"
+//
+// -mamdgpu-debugger-reserve-trap-regs
+// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri 
-mamdgpu-debugger-reserve-trap-regs %s -o 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-RESERVE-TRAP-REGS 
%s
+// CHECK-MAMDGPU-DEBUGGER-RESERVE-TRAP-REGS: "-target-feature" 
"+amdgpu-debugger-reserve-trap-regs"
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -2391,6 +2391,22 @@
   handleTargetFeaturesGroup(Args, Features, 
options::OPT_m_wasm_Features_Group);
 }
 
+static void getAMDGPUTargetFeatures(const Driver &D, const ArgList &Args,
+std::vector &Features) {
+  if (const Arg *dAbi = Args.getLastArg(options::OPT_mamdgpu_debugger_abi)) {
+StringRef value = dAbi->getValue();
+if (value == "1.0") {
+  Features.push_back("+amdgpu-debugger-insert-nops");
+  Features.push_back("+amdgpu-debugger-reserve-trap-regs");
+} else {
+  D.Diag(diag::err_drv_clang_unsupported) << dAbi->getAsString(Args);
+}
+  }
+
+  handleTargetFeaturesGroup(
+Args, Features, options::OPT_m_amdgpu_Features_Group);
+}
+
 static void getTargetFeatures(const ToolChain &TC, const llvm::Triple &Triple,
   const ArgList &Args, ArgStringList &CmdArgs,
   bool ForAS) {
@@ -2436,6 +2452,10 @@
   case llvm::Triple::wasm64:
 getWebAssemblyTargetFeatures(Args, Features);
 break;
+  case llvm::Triple::r600:
+  case llvm::Triple::amdgcn:
+getAMDGPUTargetFeatures(D, Args, Features);
+break;
   }
 
   // Find the last of each feature.
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -95,6 +95,8 @@
Group;
 def m_wasm_Features_Group : OptionGroup<"">,
 Group;
+def m_amdgpu_Features_Group : OptionGroup<"">,
+  Group;
 
 def m_libc_Group  : OptionGroup<"">, Group;
 def u_Group   : OptionGroup<"">;
@@ -1438,6 +1440,16 @@
 def msimd128 : Flag<["-"], "msimd128">, Group;
 def mno_simd128 : Flag<["-"], "mno-simd128">, Group;
 
+def mamdgpu_debugger_abi : Joined<["-"], "mamdgpu-debugger-abi=">,
+  Flags<[HelpHidden]>,
+  Group,
+  HelpText<"Generate additional code for specified  of debugger ABI 
(AMDGPU only)">,
+  MetaVarName<"">;
+def mamdgpu_debugger_insert_nops : Flag<["-"], "mamdgpu-debugger-insert-nops">,
+  Group;
+def mamdgpu_debugger_reserve_trap_regs : Flag<["-"], 
"mamdgpu-debugger-reserve-trap-regs">,
+  Group;
+
 def mvsx : Flag<["-"], "mvsx">, Group;
 def mno_vsx : Flag<["-"], "mno-vsx">, Group;
 def mpower8_vector : Flag<["-"], "mpower8-vector">,


Index: test/Driver/amdgpu-features.c
===
--- test/Driver/amdgpu-features.c
+++ test/Driver/amdgpu-features.c
@@ -0,0 +1,21 @@
+// Check handling of AMDGPU target features.
+//
+// -mamdgpu-debugger-abi=0.0
+// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri -mamdgpu-d

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

2016-04-03 Thread Richard Smith via cfe-commits 
rsmith added inline comments.


Comment at: lib/Lex/PPMacroExpansion.cpp:1430-1432
@@ +1429,5 @@
+Preprocessor &PP,
+int(*Op)(Token &Tok,
+ Preprocessor &PP,
+ bool &HasLexedNextTok)) {
+  // Parse the initial '('.

Use `llvm::function_ref` here instead, and don't pass a `Preprocessor` into the 
function (instead, capture it in a lambda).


Comment at: lib/Lex/PPMacroExpansion.cpp:1456-1457
@@ +1455,4 @@
+
+// Parse next non-comment, non-annotation token.
+do PP.LexUnexpandedNonComment(Tok); while (Tok.isAnnotation());
+

If we get an annotation token here, we should reject it, not silently ignore 
it. Also, we shouldn't see comment tokens here (we shouldn't be doing macro 
expansion with comments enabled); you should call `LexUnexpandedToken` rather 
than `LexUnexpandedNonComment`.


Comment at: lib/Lex/PPMacroExpansion.cpp:1481-1484
@@ +1480,6 @@
+  auto Diag = PP.Diag(Tok.getLocation(), 
diag::err_pp_unexpected_after);
+  if (IdentifierInfo *LastII = LastTok.getIdentifierInfo())
+Diag << LastII;
+  else
+Diag << LastTok.getKind();
+  Diag << tok::l_paren << LastTok.getLocation();

The only way we can get here without already having a value or producing a 
diagnostic is if this is the first token inside the parens. So this will always 
say "unexpected '(' after '('".

I think it would be better to always `break` here after incrementing 
`ParenDepth` (even when `!Result.hasValue()`), and let `Op` produce the 
relevant diagnostic for this case.


Comment at: lib/Lex/PPMacroExpansion.cpp:1519
@@ +1518,3 @@
+
+// Diagnose expected ')'.
+if (!SuppressDiagnostic) {

expected -> missing


http://reviews.llvm.org/D17149



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17963: [OPENMP] Codegen for teams directive for NVPTX

2016-04-03 Thread Alexey Bataev via cfe-commits 
ABataev added a comment.

LG


Repository:
  rL LLVM

http://reviews.llvm.org/D17963



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18110: [OpenMP] Fix SEMA bug in the capture of global variables in template functions.

2016-04-03 Thread Alexey Bataev via cfe-commits 
ABataev added inline comments.


Comment at: lib/Sema/SemaOpenMP.cpp:816-822
@@ -801,6 +815,9 @@
+
+  // A DSA refers to this captured region if the parent contexts match.
+  auto *ParentContext = RSI->TheCapturedDecl->getParent();
   for (auto I = Stack.rbegin(), EE = Stack.rend(); I != EE; ++I)
-if (I->CurScope == S)
+if (I->ParentDeclContext == ParentContext)
   return I->Directive;
   return OMPD_unknown;
 }
 

sfantao wrote:
> ABataev wrote:
> > sfantao wrote:
> > > ABataev wrote:
> > > > Actually, I think, we need to rework the whole IsOpenMPCapturedByRef() 
> > > > function. I'm not sure that it is even required. It looks like some 
> > > > optimization hack in the frontend. I'm against any not-necessary 
> > > > optimizations in frontend. If scalar value is a firstprivate, it must 
> > > > be handled in codegen, not by handling it by copy.
> > > 'IsOpenMPCapturedByRef' goal is to change the signature of the outlined 
> > > function. We don't want to have reference types in target region 
> > > arguments unless they are really required. We have seen performance being 
> > > greatly affected just because of that. Of course a consequence of this is 
> > > to have variables that become first private.
> > > 
> > > The current implementation of OpenMP firstprivate doesn't change the the 
> > > function signatures, only the codegen inside the region is affected. So, 
> > > this is not a complete overlap.
> > > 
> > > With this, I am not saying that this cannot be done in codegen. The 
> > > reason I am doing it here is your initial guideline that we should 
> > > attempt to fix most of the things in Sema and avoid complicating codegen 
> > > which is a more sensitive part.
> > > 
> > > Doing this in Codegen would require changing the implementation of 
> > > `GenerateOpenMPCapturedVars` and `GenerateOpenMPCapturedStmtFunction`. We 
> > > wouldn't need the tracking of the context anymore (checking the directive 
> > > would be sufficient), but we would still need to see what't in the map 
> > > clauses.
> > > 
> > > So, do you think we should change this?
> > I think we should not dot it at all for now. Performance is an important 
> > thing, of course, but at first we must have just working solution and only 
> > after that we will think about performance.
> > Early optimization leads to many troubles. The code becomes very hard to 
> > understand. It just increases the time of the development.
> > I suggest to remove all optimizations for now and emit the code as is, 
> > before we have a working solution.
> > Also, this function may lead to incorrect codegen. You don't check that 
> > CapturedRegion is actually an OpenMP region. If we have non-OpenMP cpatured 
> > region between OpenMP regions it will lead to incorrect code.
> I don't agree. As you know, several components on the codegen depend on how 
> things are captured. So in my view, identifying the best possible 
> implementation (including performance-wise) as early as possible is in our 
> best interest to avoid major refactoring down the road.
> 
> All the offloading implementation that we have working today in the trunk is 
> relying on this already. So, by rolling back to capture by reference 
> everything, will require to refactor all the offloading 
> code/patches/regression tests just to have to redo them again in the near 
> future because of the issues we have identified already. 
> 
> I'd rather spend that effort to have the capture by copy to work in a 
> acceptable way and I can move this logic to codegen if you think that is the 
> best way to do it. Note that will require checking the expressions of the map 
> clause in `GenerateOpenMPCapturedVars` and 
> `GenerateOpenMPCapturedStmtFunction`, something that Sema will have to do 
> anyway.
> 
> Let me know your thoughts. 
> 
> 
I don't like this series of patches because they are very intrusive. They add a 
lot of code, that is very hard to read and understand, very hard to refactor. 
That's why I'd prefer to see a simple solution at first and only after that try 
to optimize something.
We had the same problems with the performance of non-offloading part. But the 
very first thing that must be implemented - functionality. Optmization, 
performance etc. is the second step. It is better to add optimization only 
after the basic functionality is implemented.
So, yes, I think you must remove all optimizations for now and start from basic 
implementation.
I don't like the idea of adding new fields. tryCaptureVariable() function has 
variable OpenMPLevel. I think it can be used for proper syncing between 
CapturedRegionInfo and OpenMP region. But I'd prefer to see all these stuff 
only after the main part of codegen for target directive is implemented


http://reviews.llvm.org/D18110



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18110: [OpenMP] Fix SEMA bug in the capture of global variables in template functions.

2016-04-03 Thread Alexey Bataev via cfe-commits 
Carlo,
Again you're talking about performance loss. I don't think that 
performance loss violates some parsing/semantic/operation rules. If we 
won't pass the argument by value the code will not work properly? OpenMP 
4.5 does not specify anything about passing firstprivate args by value 
or by reference.
I'm not against optimization. But what I'm talking about is: let's start 
with basic support and then, we the basic part of the code is committed 
and the design/architecture of the solution is stable enough, we will 
work on optimizations. Because early optimization does nopt allow to 
simplify design of the whole OpenMP implementation and it leads to the 
code that very hard to modify, to read and to mantain. We already have a 
lot of troubles with Sema part, it is very complex, not very good 
structured. I don't want to have the same troubles with the codegen part 
also.

Best regards,
Alexey Bataev
=
Software Engineer
Intel Compiler Team

01.04.2016 19:10, Carlo Bertolli пишет:
> carlo.bertolli added a comment.
>
> Hi
>
> If I understand correctly the problem, I would like to add something on top 
> of Samuel's comment.
> My understanding is that Alexey is suggesting that we pass a reference type 
> to kernels for every pointer input parameter, regardless of the actual type 
> and data sharing attribute of the related variable. Ignore the remained of 
> this comment if this is not the case.
>
> In my viewpoint, this violates the basic logic for which target firstprivate 
> was introduced in the OpenMP specification: to avoid having to pass to GPU 
> kernels a reference to every pointer variable. In fact, having to pass a 
> reference results in ptxas generating an additional register for each input 
> parameter. We saw significant performance differences in this respect, and 
> reported about this in a paper at SC15. This has been reported by various 
> members of the OpenMP committee multiple times as the reason why the target 
> firstprivate logic is defined the way it is.
>
> In conclusion, I do not see this patch as an optimization, but as a way of 
> correctly implementing what the OpenMP specification clearly state. Any other 
> implementation is wrong, not a simpler, non optimized version.
>
>
> http://reviews.llvm.org/D18110
>
>
>

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits